Resolve comments

Author: mohamed-essam

client/internal/engine.go

@@ -306,6 +306,10 @@ func (e *Engine) Stop() error {
 		e.srWatcher.Close()
 	}
 
+	if e.updateManager != nil {
+		e.updateManager.Stop()
+	}
+
 	e.statusRecorder.ReplaceOfflinePeers([]peer.State{})
 	e.statusRecorder.UpdateDNSStates([]peer.NSGroupState{})
 	e.statusRecorder.UpdateRelayStates([]relay.ProbeResult{})

client/internal/updatemanager/manager.go

@@ -1,6 +1,8 @@
 package updatemanager
 
 import (
+	"context"
+	"fmt"
 	"github.com/netbirdio/netbird/client/internal/peer"
 	cProto "github.com/netbirdio/netbird/client/proto"
 	"io"
@@ -16,7 +18,14 @@ import (
 	"github.com/netbirdio/netbird/version"
 )
 
+const (
+	latestVersion     = "latest"
+	disableAutoUpdate = "disabled"
+)
+
 type UpdateManager struct {
+	ctx            context.Context
+	cancel         context.CancelFunc
 	version        string
 	update         *version.Update
 	lastTrigger    time.Time
@@ -25,33 +34,41 @@ type UpdateManager struct {
 
 func NewUpdateManager(statusRecorder *peer.Status) *UpdateManager {
 	update := version.NewUpdate("nb/client")
+	ctx, cancel := context.WithCancel(context.Background())
 	manager := &UpdateManager{
 		update:         update,
 		lastTrigger:    time.Now().Add(-10 * time.Minute),
 		statusRecorder: statusRecorder,
+		ctx:            ctx,
+		cancel:         cancel,
+		version:        disableAutoUpdate,
 	}
-	manager.version = "disabled"
 	update.SetDaemonVersion(version.NetbirdVersion())
 	update.SetOnUpdateListener(manager.CheckForUpdates)
 	return manager
 }
 
 func (u *UpdateManager) SetVersion(v string) {
 	if u.version != v {
-		log.Errorf("############## Version set to %s", v)
+		log.Tracef("Auto-update verstion set to %s", v)
 		u.version = v
 		go u.CheckForUpdates()
 	}
 }
 
+func (u *UpdateManager) Stop() {
+	u.update.StopWatch()
+	u.cancel()
+}
+
 func (u *UpdateManager) CheckForUpdates() {
-	if u.version == "disabled" {
+	if u.version == disableAutoUpdate {
 		log.Trace("Skipped checking for updates, auto-update is disabled")
 		return
 	}
 	currentVersionString := version.NetbirdVersion()
 	updateVersionString := u.version
-	if updateVersionString == "latest" || updateVersionString == "" {
+	if updateVersionString == latestVersion || updateVersionString == "" {
 		if u.update.LatestAvailable == nil {
 			log.Tracef("Latest version not fetched yet")
 			return
@@ -89,27 +106,39 @@ func (u *UpdateManager) CheckForUpdates() {
 	}
 }
 
-func downloadFileToTemporaryDir(fileURL string) (string, error) { //nolint:unused
+func downloadFileToTemporaryDir(ctx context.Context, fileURL string) (string, error) { //nolint:unused
 	tempDir, err := os.MkdirTemp("", "netbird-installer-*")
 	if err != nil {
-		return "", err
+		return "", fmt.Errorf("error creating temporary directory: %w", err)
 	}
 	fileNameParts := strings.Split(fileURL, "/")
 	out, err := os.Create(filepath.Join(tempDir, fileNameParts[len(fileNameParts)-1]))
 	if err != nil {
-		return "", err
+		return "", fmt.Errorf("error creating temporary file: %w", err)
 	}
-	defer out.Close()
+	defer func() {
+		if err := out.Close(); err != nil {
+			log.Errorf("Error closing temporary file: %v", err)
+		}
+	}()
 
-	resp, err := http.Get(fileURL)
+	req, err := http.NewRequestWithContext(ctx, "GET", fileURL, nil)
+	if err != nil {
+		return "", fmt.Errorf("error creating file download request: %w", err)
+	}
+	resp, err := http.DefaultClient.Do(req)
 	if err != nil {
-		return "", err
+		return "", fmt.Errorf("error downloading file: %w", err)
 	}
-	defer resp.Body.Close()
+	defer func() {
+		if err := resp.Body.Close(); err != nil {
+			log.Errorf("Error closing response body: %v", err)
+		}
+	}()
 
 	_, err = io.Copy(out, resp.Body)
 	if err != nil {
-		return "", err
+		return "", fmt.Errorf("error downloading file: %w", err)
 	}
 
 	log.Tracef("Downloaded update file to %s", out.Name())

client/internal/updatemanager/update_darwin.go

@@ -4,9 +4,9 @@ package updatemanager
 
 import (
 	"fmt"
-	log "github.com/sirupsen/logrus"
 	"os"
 	"os/exec"
+	"os/user"
 	"runtime"
 	"strings"
 	"syscall"
@@ -27,9 +27,9 @@ func (u *UpdateManager) triggerUpdate(targetVersion string) error {
 	// Installed using pkg file
 	url := strings.ReplaceAll(pkgDownloadURL, "%version", targetVersion)
 	url = strings.ReplaceAll(url, "%arch", runtime.GOARCH)
-	path, err := downloadFileToTemporaryDir(url)
+	path, err := downloadFileToTemporaryDir(u.ctx, url)
 	if err != nil {
-		return err
+		return fmt.Errorf("error downloading update file: %w", err)
 	}
 
 	volume := "/"
@@ -44,7 +44,7 @@ func (u *UpdateManager) triggerUpdate(targetVersion string) error {
 
 	err = cmd.Start()
 	if err != nil {
-		return err
+		return fmt.Errorf("error running pkg file: %w", err)
 	}
 	err = cmd.Process.Release()
 
@@ -56,43 +56,42 @@ func (u *UpdateManager) updateHomeBrew() error {
 	// To find out which user installed NetBird using HomeBrew we can check the owner of our brew tap directory
 	fileInfo, err := os.Stat("/opt/homebrew/Library/Taps/netbirdio/homebrew-tap/")
 	if err != nil {
-		return err
+		return fmt.Errorf("error getting homebrew installation path info: %w", err)
 	}
 
 	fileSysInfo, ok := fileInfo.Sys().(*syscall.Stat_t)
 	if !ok {
-		return fmt.Errorf("Error checking file owner, sysInfo type is %T not *syscall.Stat_t", fileInfo.Sys())
+		return fmt.Errorf("error checking file owner, sysInfo type is %T not *syscall.Stat_t", fileInfo.Sys())
 	}
 
-	// Get user name from UID
-	cmd := exec.Command("id", "-nu", fmt.Sprintf("%d", fileSysInfo.Uid))
-	out, err := cmd.CombinedOutput()
+	// Get username from UID
+	installer, err := user.LookupId(fmt.Sprintf("%d", fileSysInfo.Uid))
 	if err != nil {
-		return err
+		return fmt.Errorf("error looking up brew installer user: %w", err)
 	}
-	userName := strings.TrimSpace(string(out))
-
+	userName := installer.Name
 	// Get user HOME, required for brew to run correctly
 	// https://github.com/Homebrew/brew/issues/15833
-	cmd = exec.Command("sudo", "-u", userName, "sh", "-c", "echo $HOME")
-	out, err = cmd.CombinedOutput()
-	if err != nil {
-		return err
-	}
-
-	homeDir := strings.TrimSpace(string(out))
+	homeDir := installer.HomeDir
 	// Homebrew does not support installing specific versions
 	// Thus it will always update to latest and ignore targetVersion
-	cmd = exec.Command("sudo", "-u", userName, "/opt/homebrew/bin/brew", "upgrade", "netbirdio/tap/netbird")
+	upgradeArgs := []string{"-u", userName, "/opt/homebrew/bin/brew", "upgrade", "netbirdio/tap/netbird"}
+	// Check if netbird-ui is installed
+	cmd := exec.Command("brew", "info", "--json", "netbirdio/tap/netbird-ui")
+	err = cmd.Run()
+	if err == nil {
+		// netbird-ui is installed
+		upgradeArgs = append(upgradeArgs, "netbirdio/tap/netbird-ui")
+	}
+	cmd = exec.Command("sudo", upgradeArgs...)
 	cmd.Env = append(cmd.Env, "HOME="+homeDir)
 
 	// Homebrew upgrade doesn't restart the client on its own
 	// So we have to wait for it to finish running and ensure it's done
 	// And then basically restart the netbird service
-	out, err = cmd.CombinedOutput()
+	err = cmd.Run()
 	if err != nil {
-		log.Errorf("Error running brew upgrade, output: %v", string(out))
-		return err
+		return fmt.Errorf("error running brew upgrade: %w", err)
 	}
 
 	currentPID := os.Getpid()
@@ -101,9 +100,15 @@ func (u *UpdateManager) updateHomeBrew() error {
 	// This is a workaround since attempting to restart using launchctl will kill the service and die before starting
 	// the service again as it's a child process
 	// using SigTerm should ensure a clean shutdown
-	cmd = exec.Command("kill", "-15", fmt.Sprintf("%d", currentPID))
-	err = cmd.Run()
+	process, err := os.FindProcess(currentPID)
+	if err != nil {
+		return fmt.Errorf("error finding current process: %w", err)
+	}
+	err = process.Signal(syscall.SIGTERM)
+	if err != nil {
+		return fmt.Errorf("error sending SIGTERM to current process: %w", err)
+	}
 	// We're dying now, which should restart us
 
-	return err
+	return nil
 }

client/internal/updatemanager/update_windows.go

@@ -20,7 +20,7 @@ func (u *UpdateManager) triggerUpdate(targetVersion string) error {
 	k, err := registry.OpenKey(registry.LOCAL_MACHINE, `SOFTWARE\Microsoft\Windows\CurrentVersion\App Paths\Netbird`, registry.QUERY_VALUE)
 	if err != nil && strings.Contains(err.Error(), "system cannot find the file specified") {
 		// Installed using MSI installer
-		path, err := downloadFileToTemporaryDir(strings.ReplaceAll(msiDownloadURL, "%s", targetVersion))
+		path, err := downloadFileToTemporaryDir(u.ctx, strings.ReplaceAll(msiDownloadURL, "%s", targetVersion))
 		if err != nil {
 			return err
 		}
@@ -36,7 +36,7 @@ func (u *UpdateManager) triggerUpdate(targetVersion string) error {
 	}
 
 	// Installed using EXE installer
-	path, err := downloadFileToTemporaryDir(strings.ReplaceAll(exeDownloadURL, "%s", targetVersion))
+	path, err := downloadFileToTemporaryDir(u.ctx, strings.ReplaceAll(exeDownloadURL, "%s", targetVersion))
 	if err != nil {
 		return err
 	}