From 70f2097fff7b9807428bd9731e224734a9a70476 Mon Sep 17 00:00:00 2001 From: riccardom Date: Tue, 16 Jun 2026 16:44:38 +0200 Subject: [PATCH] config becomes start/run arg so we are sure it gets updated on any (re)start This is because we now have the supervisor not destroyed over time Still intermediate step where server.go regenerates the client entirely. --- client/android/client.go | 8 ++--- client/cmd/up.go | 4 +-- client/embed/embed.go | 4 +-- client/internal/connect.go | 39 +++++++++++----------- client/internal/connect_android_default.go | 4 ++- client/internal/connect_android_embed.go | 4 ++- client/internal/connect_lifecycle.go | 22 +++++++----- client/ios/NetBirdSDK/client.go | 4 +-- client/server/mdm.go | 3 +- client/server/server.go | 27 +++++++++++---- client/server/server_connect_test.go | 2 +- client/server/server_test.go | 3 +- 12 files changed, 73 insertions(+), 51 deletions(-) diff --git a/client/android/client.go b/client/android/client.go index 99ccdf393..3a0a02c0b 100644 --- a/client/android/client.go +++ b/client/android/client.go @@ -151,9 +151,9 @@ func (c *Client) Run(platformFiles PlatformFiles, urlOpener URLOpener, isAndroid // todo do not throw error in case of cancelled context ctx = internal.CtxInitState(ctx) - connectClient := internal.NewConnectClient(ctx, cfg, c.recorder) + connectClient := internal.NewConnectClient(ctx, c.recorder) c.setState(cfg, cacheDir, connectClient) - return connectClient.RunOnAndroid(c.tunAdapter, c.iFaceDiscover, c.networkChangeListener, slices.Clone(dns.items), dnsReadyListener, stateFile, cacheDir) + return connectClient.RunOnAndroid(cfg, c.tunAdapter, c.iFaceDiscover, c.networkChangeListener, slices.Clone(dns.items), dnsReadyListener, stateFile, cacheDir) } // RunWithoutLogin we apply this type of run function when the backed has been started without UI (i.e. after reboot). @@ -186,9 +186,9 @@ func (c *Client) RunWithoutLogin(platformFiles PlatformFiles, dns *DNSList, dnsR // todo do not throw error in case of cancelled context ctx = internal.CtxInitState(ctx) - connectClient := internal.NewConnectClient(ctx, cfg, c.recorder) + connectClient := internal.NewConnectClient(ctx, c.recorder) c.setState(cfg, cacheDir, connectClient) - return connectClient.RunOnAndroid(c.tunAdapter, c.iFaceDiscover, c.networkChangeListener, slices.Clone(dns.items), dnsReadyListener, stateFile, cacheDir) + return connectClient.RunOnAndroid(cfg, c.tunAdapter, c.iFaceDiscover, c.networkChangeListener, slices.Clone(dns.items), dnsReadyListener, stateFile, cacheDir) } // Stop the internal client and free the resources diff --git a/client/cmd/up.go b/client/cmd/up.go index cabd0aacf..79e47ad99 100644 --- a/client/cmd/up.go +++ b/client/cmd/up.go @@ -202,10 +202,10 @@ func runInForegroundMode(ctx context.Context, cmd *cobra.Command, activeProf *pr r := peer.NewRecorder(config.ManagementURL.String()) r.GetFullStatus() - connectClient := internal.NewConnectClient(ctx, config, r) + connectClient := internal.NewConnectClient(ctx, r) SetupDebugHandler(ctx, config, r, connectClient, "") - return connectClient.Run(nil, util.FindFirstLogPath(logFiles)) + return connectClient.Run(config, nil, util.FindFirstLogPath(logFiles)) } func runInDaemonMode(ctx context.Context, cmd *cobra.Command, pm *profilemanager.ProfileManager, activeProf *profilemanager.Profile, profileSwitched bool) error { diff --git a/client/embed/embed.go b/client/embed/embed.go index 0e8991be2..e0d123a4e 100644 --- a/client/embed/embed.go +++ b/client/embed/embed.go @@ -264,7 +264,7 @@ func (c *Client) Start(startCtx context.Context) error { if err, _ := authClient.Login(ctx, c.setupKey, c.jwtToken); err != nil { return fmt.Errorf("login: %w", err) } - client := internal.NewConnectClient(ctx, c.config, c.recorder) + client := internal.NewConnectClient(ctx, c.recorder) client.SetSyncResponsePersistence(true) // either startup error (permanent backoff err) or nil err (successful engine up) @@ -272,7 +272,7 @@ func (c *Client) Start(startCtx context.Context) error { run := make(chan struct{}) clientErr := make(chan error, 1) go func() { - if err := client.Run(run, ""); err != nil { + if err := client.Run(c.config, run, ""); err != nil { clientErr <- err } }() diff --git a/client/internal/connect.go b/client/internal/connect.go index b20bff289..8a99bb1f9 100644 --- a/client/internal/connect.go +++ b/client/internal/connect.go @@ -50,11 +50,10 @@ import ( // androidRunOverride is set on Android to inject mobile dependencies // when using embed.Client (which calls Run() with empty MobileDependency). -var androidRunOverride func(c *ConnectClient, runningChan chan struct{}, logPath string) error +var androidRunOverride func(c *ConnectClient, config *profilemanager.Config, runningChan chan struct{}, logPath string) error type ConnectClient struct { ctx context.Context - config *profilemanager.Config statusRecorder *peer.Status engine *Engine @@ -71,12 +70,10 @@ type ConnectClient struct { func NewConnectClient( ctx context.Context, - config *profilemanager.Config, statusRecorder *peer.Status, ) *ConnectClient { c := &ConnectClient{ ctx: ctx, - config: config, statusRecorder: statusRecorder, engineMutex: sync.Mutex{}, } @@ -89,15 +86,16 @@ func (c *ConnectClient) SetUpdateManager(um *updater.Manager) { } // Run with main logic. -func (c *ConnectClient) Run(runningChan chan struct{}, logPath string) error { +func (c *ConnectClient) Run(config *profilemanager.Config, runningChan chan struct{}, logPath string) error { if androidRunOverride != nil { - return androidRunOverride(c, runningChan, logPath) + return androidRunOverride(c, config, runningChan, logPath) } - return c.sup.start(MobileDependency{}, runningChan, logPath) + return c.sup.start(config, MobileDependency{}, runningChan, logPath) } // RunOnAndroid with main logic on mobile system func (c *ConnectClient) RunOnAndroid( + config *profilemanager.Config, tunAdapter device.TunAdapter, iFaceDiscover stdnet.ExternalIFaceDiscover, networkChangeListener listener.NetworkChangeListener, @@ -116,10 +114,11 @@ func (c *ConnectClient) RunOnAndroid( StateFilePath: stateFilePath, TempDir: cacheDir, } - return c.sup.start(mobileDependency, nil, "") + return c.sup.start(config, mobileDependency, nil, "") } func (c *ConnectClient) RunOniOS( + config *profilemanager.Config, fileDescriptor int32, networkChangeListener listener.NetworkChangeListener, dnsManager dns.IosDnsManager, @@ -134,12 +133,12 @@ func (c *ConnectClient) RunOniOS( DnsManager: dnsManager, StateFilePath: stateFilePath, } - return c.sup.start(mobileDependency, nil, "") + return c.sup.start(config, mobileDependency, nil, "") } // run executes a single client run. runCtx is owned by the supervisor: cancelling // it tears the run down (it is the parent of the per-attempt engine context). -func (c *ConnectClient) run(runCtx context.Context, mobileDependency MobileDependency, runningChan chan struct{}, logPath string) error { +func (c *ConnectClient) run(runCtx context.Context, config *profilemanager.Config, mobileDependency MobileDependency, runningChan chan struct{}, logPath string) error { defer func() { if r := recover(); r != nil { rec := c.statusRecorder @@ -203,18 +202,18 @@ func (c *ConnectClient) run(runCtx context.Context, mobileDependency MobileDepen }() wrapErr := state.Wrap - myPrivateKey, err := wgtypes.ParseKey(c.config.PrivateKey) + myPrivateKey, err := wgtypes.ParseKey(config.PrivateKey) if err != nil { - log.Errorf("failed parsing Wireguard key %s: [%s]", c.config.PrivateKey, err.Error()) + log.Errorf("failed parsing Wireguard key %s: [%s]", config.PrivateKey, err.Error()) return wrapErr(err) } var mgmTlsEnabled bool - if c.config.ManagementURL.Scheme == "https" { + if config.ManagementURL.Scheme == "https" { mgmTlsEnabled = true } - publicSSHKey, err := ssh.GeneratePublicKey([]byte(c.config.SSHKey)) + publicSSHKey, err := ssh.GeneratePublicKey([]byte(config.SSHKey)) if err != nil { return err } @@ -262,8 +261,8 @@ func (c *ConnectClient) run(runCtx context.Context, mobileDependency MobileDepen cancel() }() - log.Debugf("connecting to the Management service %s", c.config.ManagementURL.Host) - mgmClient, err := mgm.NewClient(engineCtx, c.config.ManagementURL.Host, myPrivateKey, mgmTlsEnabled) + log.Debugf("connecting to the Management service %s", config.ManagementURL.Host) + mgmClient, err := mgm.NewClient(engineCtx, config.ManagementURL.Host, myPrivateKey, mgmTlsEnabled) if err != nil { return wrapErr(gstatus.Errorf(codes.FailedPrecondition, "failed connecting to Management Service : %s", err)) } @@ -280,7 +279,7 @@ func (c *ConnectClient) run(runCtx context.Context, mobileDependency MobileDepen } c.clientMetrics.UpdateAgentInfo(agentInfo, myPrivateKey.PublicKey().String()) - log.Debugf("connected to the Management service %s", c.config.ManagementURL.Host) + log.Debugf("connected to the Management service %s", config.ManagementURL.Host) defer func() { if err = mgmClient.Close(); err != nil { log.Warnf("failed to close the Management service client %v", err) @@ -289,7 +288,7 @@ func (c *ConnectClient) run(runCtx context.Context, mobileDependency MobileDepen // connect (just a connection, no stream yet) and login to Management Service to get an initial global Netbird config loginStarted := time.Now() - loginResp, err := loginToManagement(engineCtx, mgmClient, publicSSHKey, c.config) + loginResp, err := loginToManagement(engineCtx, mgmClient, publicSSHKey, config) if err != nil { c.clientMetrics.RecordLoginDuration(engineCtx, time.Since(loginStarted), false) log.Debug(err) @@ -349,7 +348,7 @@ func (c *ConnectClient) run(runCtx context.Context, mobileDependency MobileDepen } peerConfig := loginResp.GetPeerConfig() - engineConfig, err := createEngineConfig(myPrivateKey, c.config, peerConfig, logPath) + engineConfig, err := createEngineConfig(myPrivateKey, config, peerConfig, logPath) if err != nil { log.Error(err) return wrapErr(err) @@ -393,7 +392,7 @@ func (c *ConnectClient) run(runCtx context.Context, mobileDependency MobileDepen c.engine = engine c.engineMutex.Unlock() - if err := engine.Start(loginResp.GetNetbirdConfig(), c.config.ManagementURL); err != nil { + if err := engine.Start(loginResp.GetNetbirdConfig(), config.ManagementURL); err != nil { log.Errorf("error while starting Netbird Connection Engine: %s", err) return wrapErr(err) } diff --git a/client/internal/connect_android_default.go b/client/internal/connect_android_default.go index b05e91fec..e22c52ace 100644 --- a/client/internal/connect_android_default.go +++ b/client/internal/connect_android_default.go @@ -7,6 +7,7 @@ import ( "github.com/netbirdio/netbird/client/internal/dns" "github.com/netbirdio/netbird/client/internal/listener" + "github.com/netbirdio/netbird/client/internal/profilemanager" "github.com/netbirdio/netbird/client/internal/stdnet" ) @@ -64,8 +65,9 @@ func init() { // dependencies so the engine's existing Android code paths work unchanged. // Applications that need P2P ICE or real DNS should replace this by // setting androidRunOverride before calling Start(). - androidRunOverride = func(c *ConnectClient, runningChan chan struct{}, logPath string) error { + androidRunOverride = func(c *ConnectClient, config *profilemanager.Config, runningChan chan struct{}, logPath string) error { return c.runOnAndroidEmbed( + config, noopIFaceDiscover{}, noopNetworkChangeListener{}, []netip.AddrPort{}, diff --git a/client/internal/connect_android_embed.go b/client/internal/connect_android_embed.go index c96a0b41d..c91937759 100644 --- a/client/internal/connect_android_embed.go +++ b/client/internal/connect_android_embed.go @@ -7,6 +7,7 @@ import ( "github.com/netbirdio/netbird/client/internal/dns" "github.com/netbirdio/netbird/client/internal/listener" + "github.com/netbirdio/netbird/client/internal/profilemanager" "github.com/netbirdio/netbird/client/internal/stdnet" ) @@ -15,6 +16,7 @@ import ( // It provides complete MobileDependency so the engine's existing // Android code paths work unchanged. func (c *ConnectClient) runOnAndroidEmbed( + config *profilemanager.Config, iFaceDiscover stdnet.ExternalIFaceDiscover, networkChangeListener listener.NetworkChangeListener, dnsAddresses []netip.AddrPort, @@ -28,5 +30,5 @@ func (c *ConnectClient) runOnAndroidEmbed( HostDNSAddresses: dnsAddresses, DnsReadyListener: dnsReadyListener, } - return c.sup.start(mobileDependency, runningChan, logPath) + return c.sup.start(config, mobileDependency, runningChan, logPath) } diff --git a/client/internal/connect_lifecycle.go b/client/internal/connect_lifecycle.go index 4ad010253..090acd701 100644 --- a/client/internal/connect_lifecycle.go +++ b/client/internal/connect_lifecycle.go @@ -3,6 +3,8 @@ package internal import ( "context" "errors" + + "github.com/netbirdio/netbird/client/internal/profilemanager" ) // errAlreadyRunning is returned when a start is requested while a run is already @@ -24,6 +26,7 @@ const ( // - for opStop it receives nil once the in-flight run has fully unwound. type lifecycleCmd struct { op lifecycleOp + config *profilemanager.Config mobileDep MobileDependency runningChan chan struct{} logPath string @@ -36,8 +39,9 @@ type runEndResult struct { err error } -// runFunc executes a single client run bound to the supervisor-owned context. -type runFunc func(ctx context.Context, mobileDep MobileDependency, runningChan chan struct{}, logPath string) error +// runFunc executes a single client run bound to the supervisor-owned context, +// with the config supplied by the start request. +type runFunc func(ctx context.Context, config *profilemanager.Config, mobileDep MobileDependency, runningChan chan struct{}, logPath string) error // supervisor serializes start/stop of a single client run. Every request goes // through cmdCh and is handled one at a time by the loop goroutine, so two @@ -98,10 +102,10 @@ func (s *supervisor) handleStart(cmd lifecycleCmd) { s.runCancel = cancel s.curExecOp = &cmd - go func(ctx context.Context, m MobileDependency, rc chan struct{}, lp string) { - err := s.run(ctx, m, rc, lp) + go func(ctx context.Context, cfg *profilemanager.Config, m MobileDependency, rc chan struct{}, lp string) { + err := s.run(ctx, cfg, m, rc, lp) s.runEnded <- runEndResult{err: err} - }(runCtx, cmd.mobileDep, cmd.runningChan, cmd.logPath) + }(runCtx, cmd.config, cmd.mobileDep, cmd.runningChan, cmd.logPath) } func (s *supervisor) handleStop(cmd lifecycleCmd) { @@ -150,8 +154,8 @@ func (s *supervisor) shutdown() { // startAsync enqueues a start without blocking. If done is non-nil it receives // the run's end result (or errAlreadyRunning on rejection, or the context error // on shutdown). -func (s *supervisor) startAsync(mobileDep MobileDependency, runningChan chan struct{}, logPath string, done chan error) { - cmd := lifecycleCmd{op: opStart, mobileDep: mobileDep, runningChan: runningChan, logPath: logPath, done: done} +func (s *supervisor) startAsync(config *profilemanager.Config, mobileDep MobileDependency, runningChan chan struct{}, logPath string, done chan error) { + cmd := lifecycleCmd{op: opStart, config: config, mobileDep: mobileDep, runningChan: runningChan, logPath: logPath, done: done} select { case s.cmdCh <- cmd: case <-s.ctx.Done(): @@ -161,9 +165,9 @@ func (s *supervisor) startAsync(mobileDep MobileDependency, runningChan chan str // start enqueues a start and blocks until the run terminates, preserving the // blocking contract of the legacy Run entry points. -func (s *supervisor) start(mobileDep MobileDependency, runningChan chan struct{}, logPath string) error { +func (s *supervisor) start(config *profilemanager.Config, mobileDep MobileDependency, runningChan chan struct{}, logPath string) error { done := make(chan error, 1) - s.startAsync(mobileDep, runningChan, logPath, done) + s.startAsync(config, mobileDep, runningChan, logPath, done) select { case err := <-done: return err diff --git a/client/ios/NetBirdSDK/client.go b/client/ios/NetBirdSDK/client.go index bafbb0031..80877c56f 100644 --- a/client/ios/NetBirdSDK/client.go +++ b/client/ios/NetBirdSDK/client.go @@ -161,8 +161,8 @@ func (c *Client) Run(fd int32, interfaceName string, envList *EnvList) error { c.onHostDnsFn = func([]string) {} cfg.WgIface = interfaceName - c.connectClient = internal.NewConnectClient(ctx, cfg, c.recorder) - return c.connectClient.RunOniOS(fd, c.networkChangeListener, c.dnsManager, c.stateFile) + c.connectClient = internal.NewConnectClient(ctx, c.recorder) + return c.connectClient.RunOniOS(cfg, fd, c.networkChangeListener, c.dnsManager, c.stateFile) } // Stop the internal client and free the resources diff --git a/client/server/mdm.go b/client/server/mdm.go index 0da0ec5d1..c47c211d4 100644 --- a/client/server/mdm.go +++ b/client/server/mdm.go @@ -159,8 +159,9 @@ func (s *Server) restartEngineForMDMLocked() error { s.clientRunning = true s.clientRunningChan = make(chan struct{}) s.clientGiveUpChan = make(chan struct{}) + client := s.newSessionClient(ctx) log.Info("MDM restart: spawning connectWithRetryRuns with re-resolved config") - go s.connectWithRetryRuns(ctx, config, s.statusRecorder, s.clientRunningChan, s.clientGiveUpChan) + go s.connectWithRetryRuns(ctx, client, config, s.statusRecorder, s.clientRunningChan, s.clientGiveUpChan) s.publishConfigChangedEvent("mdm") return nil } diff --git a/client/server/server.go b/client/server/server.go index 1db2aa7a2..55da13b7b 100644 --- a/client/server/server.go +++ b/client/server/server.go @@ -235,7 +235,8 @@ func (s *Server) Start() error { s.clientRunning = true s.clientRunningChan = make(chan struct{}) s.clientGiveUpChan = make(chan struct{}) - go s.connectWithRetryRuns(ctx, config, s.statusRecorder, s.clientRunningChan, s.clientGiveUpChan) + client := s.newSessionClient(ctx) + go s.connectWithRetryRuns(ctx, client, config, s.statusRecorder, s.clientRunningChan, s.clientGiveUpChan) s.publishConfigChangedEvent("startup") return nil } @@ -252,7 +253,7 @@ func (s *Server) Start() error { // of clientGiveUpChan. The defer does NOT touch s.mutex; the daemon's // "intent" (clientRunning) is maintained by the RPC handlers, not by this // goroutine. -func (s *Server) connectWithRetryRuns(ctx context.Context, profileConfig *profilemanager.Config, statusRecorder *peer.Status, runningChan chan struct{}, giveUpChan chan struct{}) { +func (s *Server) connectWithRetryRuns(ctx context.Context, client *internal.ConnectClient, profileConfig *profilemanager.Config, statusRecorder *peer.Status, runningChan chan struct{}, giveUpChan chan struct{}) { defer func() { if giveUpChan != nil { close(giveUpChan) @@ -260,7 +261,7 @@ func (s *Server) connectWithRetryRuns(ctx context.Context, profileConfig *profil }() if s.config.DisableAutoConnect { - if err := s.connect(ctx, s.config, s.statusRecorder, runningChan); err != nil { + if err := s.connect(client, s.config, runningChan); err != nil { log.Debugf("run client connection exited with error: %v", err) } log.Tracef("client connection exited") @@ -289,7 +290,7 @@ func (s *Server) connectWithRetryRuns(ctx context.Context, profileConfig *profil }() runOperation := func() error { - err := s.connect(ctx, profileConfig, statusRecorder, runningChan) + err := s.connect(client, profileConfig, runningChan) if err != nil { log.Debugf("run client connection exited with error: %v. Will retry in the background", err) return err @@ -837,7 +838,8 @@ func (s *Server) Up(callerCtx context.Context, msg *proto.UpRequest) (*proto.UpR s.clientRunningChan = make(chan struct{}) s.clientGiveUpChan = make(chan struct{}) - go s.connectWithRetryRuns(ctx, s.config, s.statusRecorder, s.clientRunningChan, s.clientGiveUpChan) + client := s.newSessionClient(ctx) + go s.connectWithRetryRuns(ctx, client, s.config, s.statusRecorder, s.clientRunningChan, s.clientGiveUpChan) s.publishConfigChangedEvent("up_rpc") s.mutex.Unlock() @@ -1752,14 +1754,25 @@ func (s *Server) GetFeatures(ctx context.Context, msg *proto.GetFeaturesRequest) return features, nil } -func (s *Server) connect(client *internal.ConnectClient, runningChan chan struct{}) error { +func (s *Server) connect(client *internal.ConnectClient, config *profilemanager.Config, runningChan chan struct{}) error { log.Tracef("running client connection") - if err := client.Run(runningChan, s.logFile); err != nil { + if err := client.Run(config, runningChan, s.logFile); err != nil { return err } return nil } +// newSessionClient builds a ConnectClient for a connection session and records +// it as the active client. Config is supplied per Run (see connect), not at +// construction, so the same client can be reused across reconnects. +func (s *Server) newSessionClient(ctx context.Context) *internal.ConnectClient { + client := internal.NewConnectClient(ctx, s.statusRecorder) + client.SetUpdateManager(s.updateManager) + client.SetSyncResponsePersistence(s.persistSyncResponse) + s.connectClient = client + return client +} + // MDM authority: when the platform-native MDM source sets a kill switch // key (regardless of true/false value), that value wins. The CLI flag // supplied at service install time is the fallback used only when the diff --git a/client/server/server_connect_test.go b/client/server/server_connect_test.go index 0c6e03a4a..7bbbe0d14 100644 --- a/client/server/server_connect_test.go +++ b/client/server/server_connect_test.go @@ -22,7 +22,7 @@ func newTestServer() *Server { } func newDummyConnectClient(ctx context.Context) *internal.ConnectClient { - return internal.NewConnectClient(ctx, nil, nil) + return internal.NewConnectClient(ctx, nil) } // TestConnectSetsClientWithMutex validates that connect() sets s.connectClient diff --git a/client/server/server_test.go b/client/server/server_test.go index 66e0fcc4c..29ba7c3fb 100644 --- a/client/server/server_test.go +++ b/client/server/server_test.go @@ -114,7 +114,8 @@ func TestConnectWithRetryRuns(t *testing.T) { t.Setenv(maxRetryTimeVar, "5s") t.Setenv(retryMultiplierVar, "1") - s.connectWithRetryRuns(ctx, config, s.statusRecorder, nil, nil) + client := s.newSessionClient(ctx) + s.connectWithRetryRuns(ctx, client, config, s.statusRecorder, nil, nil) if counter < 3 { t.Fatalf("expected counter > 2, got %d", counter) }