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.
This commit is contained in:
riccardom
2026-06-16 16:44:38 +02:00
parent befa9a879c
commit 70f2097fff
12 changed files with 73 additions and 51 deletions

View File

@@ -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

View File

@@ -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 {

View File

@@ -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
}
}()

View File

@@ -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)
}

View File

@@ -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{},

View File

@@ -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)
}

View File

@@ -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

View File

@@ -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

View File

@@ -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
}

View File

@@ -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

View File

@@ -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

View File

@@ -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)
}