From da5c245061958bb5b851f52332fc4766cf50ecdd Mon Sep 17 00:00:00 2001 From: James Otting Date: Thu, 5 Dec 2024 11:58:48 -0500 Subject: [PATCH] Fix wifi candidate selection to include non-interface-bound candidates (#49) --- cmd/viam-agent/main.go | 2 +- subsystems/provisioning/definitions.go | 2 - subsystems/provisioning/generators.go | 12 ------ subsystems/provisioning/networkmanager.go | 50 +++++++++++------------ subsystems/provisioning/networkstate.go | 35 +++++++++++++--- subsystems/provisioning/provisioning.go | 2 + subsystems/provisioning/scanning.go | 6 ++- subsystems/provisioning/setup.go | 1 + 8 files changed, 62 insertions(+), 48 deletions(-) diff --git a/cmd/viam-agent/main.go b/cmd/viam-agent/main.go index 398f547..ccccb74 100644 --- a/cmd/viam-agent/main.go +++ b/cmd/viam-agent/main.go @@ -183,7 +183,7 @@ func main() { } // wait until now when we (potentially) have a network logger to record this - globalLogger.Infof("Viam Agent Version: %s\nGit Revision: %s\n", agent.GetVersion(), agent.GetRevision()) + globalLogger.Infof("Viam Agent Version: %s Git Revision: %s", agent.GetVersion(), agent.GetRevision()) // if FastStart is set, skip updates and start viam-server immediately, then proceed as normal var fastSuccess bool diff --git a/subsystems/provisioning/definitions.go b/subsystems/provisioning/definitions.go index 152d0d2..c1134c6 100644 --- a/subsystems/provisioning/definitions.go +++ b/subsystems/provisioning/definitions.go @@ -37,8 +37,6 @@ const ( NetworkTypeWired = "wired" NetworkTypeHotspot = "hotspot" - IfNameAny = "any" - HealthCheckTimeout = time.Minute ) diff --git a/subsystems/provisioning/generators.go b/subsystems/provisioning/generators.go index 2559729..c4659f1 100644 --- a/subsystems/provisioning/generators.go +++ b/subsystems/provisioning/generators.go @@ -2,7 +2,6 @@ package provisioning import ( "encoding/binary" - "fmt" "net" "regexp" "strconv" @@ -178,14 +177,3 @@ func generateAddress(addr string) (uint32, error) { return binary.LittleEndian.Uint32(outBytes), nil } - -func genNetKey(ifName, ssid string) string { - if ifName == "" { - ifName = IfNameAny - } - - if ssid == "" { - return ifName - } - return fmt.Sprintf("%s@%s", ssid, ifName) -} diff --git a/subsystems/provisioning/networkmanager.go b/subsystems/provisioning/networkmanager.go index 81683b8..9e0c297 100644 --- a/subsystems/provisioning/networkmanager.go +++ b/subsystems/provisioning/networkmanager.go @@ -126,7 +126,7 @@ func (w *Provisioning) checkConnections() error { state, err := activeConnection.GetPropertyState() nw.mu.Lock() if err != nil { - w.logger.Error(errw.Wrapf(err, "getting state of active connection: %s", genNetKey(ifName, ssid))) + w.logger.Error(errw.Wrapf(err, "getting state of active connection: %s", w.netState.GenNetKey(ifName, ssid))) w.netState.SetActiveConn(ifName, nil) w.netState.SetActiveSSID(ifName, "") nw.connected = false @@ -221,16 +221,11 @@ func (w *Provisioning) activateConnection(ctx context.Context, ifName, ssid stri nw.mu.Lock() defer nw.mu.Unlock() - if nw.conn == nil && ssid != "" { - nw = w.netState.LockingNetwork(IfNameAny, ssid) - nw.mu.Lock() - defer nw.mu.Unlock() - if nw.conn == nil { - return errw.Errorf("no settings found for network: %s", genNetKey(ifName, ssid)) - } + if nw.conn == nil { + return errw.Errorf("no settings found for network: %s", w.netState.GenNetKey(ifName, ssid)) } - w.logger.Infof("Activating connection: %s", genNetKey(ifName, ssid)) + w.logger.Infof("Activating connection: %s", w.netState.GenNetKey(ifName, ssid)) var netDev gnm.Device if nw.netType == NetworkTypeWifi || nw.netType == NetworkTypeHotspot { @@ -253,7 +248,7 @@ func (w *Provisioning) activateConnection(ctx context.Context, ifName, ssid stri activeConnection, err := w.nm.ActivateConnection(nw.conn, netDev, nil) if err != nil { nw.lastError = err - return errw.Wrapf(err, "activating connection: %s", genNetKey(ifName, ssid)) + return errw.Wrapf(err, "activating connection: %s", w.netState.GenNetKey(ifName, ssid)) } if err := w.waitForConnect(ctx, netDev); err != nil { @@ -267,7 +262,7 @@ func (w *Provisioning) activateConnection(ctx context.Context, ifName, ssid stri nw.lastError = nil w.netState.SetActiveConn(ifName, activeConnection) - w.logger.Infof("Successfully activated connection: %s", genNetKey(ifName, ssid)) + w.logger.Infof("Successfully activated connection: %s", w.netState.GenNetKey(ifName, ssid)) if nw.netType != NetworkTypeHotspot { w.netState.SetActiveSSID(ifName, ssid) @@ -300,14 +295,14 @@ func (w *Provisioning) deactivateConnection(ifName, ssid string) error { nw.mu.Lock() defer nw.mu.Unlock() - w.logger.Infof("Deactivating connection: %s", genNetKey(ifName, ssid)) + w.logger.Infof("Deactivating connection: %s", w.netState.GenNetKey(ifName, ssid)) if err := w.nm.DeactivateConnection(activeConn); err != nil { nw.lastError = err - return errw.Wrapf(err, "deactivating connection: %s", genNetKey(ifName, ssid)) + return errw.Wrapf(err, "deactivating connection: %s", w.netState.GenNetKey(ifName, ssid)) } - w.logger.Infof("Successfully deactivated connection: %s", genNetKey(ifName, ssid)) + w.logger.Infof("Successfully deactivated connection: %s", w.netState.GenNetKey(ifName, ssid)) if ifName == w.Config().HotspotInterface { w.connState.setConnected(false) @@ -375,7 +370,7 @@ func (w *Provisioning) addOrUpdateConnection(cfg NetworkConfig) (bool, error) { return changesMade, errors.New("wifi passwords must be at least 8 characters long, or completely empty (for unsecured networks)") } - netKey := genNetKey(cfg.Interface, cfg.SSID) + netKey := w.netState.GenNetKey(cfg.Interface, cfg.SSID) nw := w.netState.LockingNetwork(cfg.Interface, cfg.SSID) nw.lastTried = time.Time{} nw.priority = cfg.Priority @@ -442,8 +437,8 @@ func (w *Provisioning) addOrUpdateConnection(cfg NetworkConfig) (bool, error) { // this doesn't error as it's not technically fatal if it fails. func (w *Provisioning) lowerMaxNetPriorities(skip string) { for _, nw := range w.netState.LockingNetworks() { - netKey := genNetKey(nw.interfaceName, nw.ssid) - if netKey == skip || netKey == genNetKey(w.Config().HotspotInterface, w.Config().hotspotSSID) || nw.priority < 999 || + netKey := w.netState.GenNetKey(nw.interfaceName, nw.ssid) + if netKey == skip || netKey == w.netState.GenNetKey(w.Config().HotspotInterface, w.Config().hotspotSSID) || nw.priority < 999 || nw.netType != NetworkTypeWifi || (nw.interfaceName != "" && nw.interfaceName != w.Config().HotspotInterface) { continue } @@ -533,7 +528,6 @@ func (w *Provisioning) getCandidates(ifName string) []string { return []string{nw.ssid} } } - return []string{} } // sort by priority @@ -607,11 +601,10 @@ func (w *Provisioning) mainLoop(ctx context.Context) { priority = 100 } cfg := NetworkConfig{ - Type: NetworkTypeWifi, - SSID: userInput.SSID, - PSK: userInput.PSK, - Priority: priority, - Interface: w.Config().HotspotInterface, + Type: NetworkTypeWifi, + SSID: userInput.SSID, + PSK: userInput.PSK, + Priority: priority, } var err error changesMade, err = w.AddOrUpdateConnection(cfg) @@ -652,7 +645,7 @@ func (w *Provisioning) mainLoop(ctx context.Context) { nw.lastError = err w.logger.Warn(err) } else { - w.logger.Error("cannot find %s in network list", genNetKey("", newSSID)) + w.logger.Error("cannot find %s in network list", w.netState.GenNetKey("", newSSID)) } nw.mu.Unlock() err = w.StartProvisioning(ctx, inputChan) @@ -691,7 +684,7 @@ func (w *Provisioning) mainLoop(ctx context.Context) { w.logger.Debugf("wifi: %t (%s), internet: %t, config present: %t", isConnected, - genNetKey(w.Config().HotspotInterface, w.netState.ActiveSSID(w.Config().HotspotInterface)), + w.netState.GenNetKey(w.Config().HotspotInterface, w.netState.ActiveSSID(w.Config().HotspotInterface)), isOnline, isConfigured, ) @@ -717,6 +710,7 @@ func (w *Provisioning) mainLoop(ctx context.Context) { w.logger.Error(err) } else { pMode = w.connState.getProvisioning() + pModeChange = w.connState.getProvisioningChange() } } } @@ -743,9 +737,11 @@ func (w *Provisioning) mainLoop(ctx context.Context) { } } + hitOfflineTimeout := lastConnectivity.Before(now.Add(time.Duration(w.cfg.OfflineTimeout)*-1)) && + pModeChange.Before(now.Add(time.Duration(w.cfg.OfflineTimeout)*-1)) // not in provisioning mode, so start it if not configured (/etc/viam.json) - // OR as long as we've been offline for at least OfflineTimeout (2 minute default) - if !isConfigured || lastConnectivity.Before(now.Add(time.Duration(w.cfg.OfflineTimeout)*-1)) { + // OR as long as we've been offline AND out of provisioning mode for at least OfflineTimeout (2 minute default) + if !isConfigured || hitOfflineTimeout { if err := w.StartProvisioning(ctx, inputChan); err != nil { w.logger.Error(err) } diff --git a/subsystems/provisioning/networkstate.go b/subsystems/provisioning/networkstate.go index 6b8c72a..76a7e8b 100644 --- a/subsystems/provisioning/networkstate.go +++ b/subsystems/provisioning/networkstate.go @@ -1,6 +1,7 @@ package provisioning import ( + "fmt" "sync" gnm "github.com/Otterverse/gonetworkmanager/v2" @@ -11,6 +12,9 @@ type networkState struct { mu sync.RWMutex logger logging.Logger + // the wifi interface to default to when no interface is specified + hotspotInterface string + // key is ssid@interface for wifi, ex: TestNetwork@wlan0 // interface may be "any" for no interface set, ex: TestNetwork@any // wired networks are just interface, ex: eth0 @@ -39,13 +43,36 @@ func NewNetworkState(logger logging.Logger) *networkState { } } +func (n *networkState) SetHotspotInterface(iface string) { + n.mu.Lock() + defer n.mu.Unlock() + n.hotspotInterface = iface +} + +func (n *networkState) GenNetKey(ifName, ssid string) string { + n.mu.Lock() + defer n.mu.Unlock() + return n.genNetKey(ifName, ssid) +} + +func (n *networkState) genNetKey(ifName, ssid string) string { + if ifName == "" && ssid != "" { + ifName = n.hotspotInterface + } + + if ssid == "" { + return ifName + } + return fmt.Sprintf("%s@%s", ssid, ifName) +} + // LockingNetwork returns a pointer to a network, wrapped in a lockable struct, so updates are persisted // Users must lock the returned network before updates. func (n *networkState) LockingNetwork(iface, ssid string) *lockingNetwork { n.mu.Lock() defer n.mu.Unlock() - id := genNetKey(iface, ssid) + id := n.genNetKey(iface, ssid) net, ok := n.network[id] if !ok { @@ -57,9 +84,7 @@ func (n *networkState) LockingNetwork(iface, ssid string) *lockingNetwork { } else { net.netType = NetworkTypeWired } - if iface != IfNameAny && iface != "" { - net.interfaceName = iface - } + net.interfaceName = iface n.logger.Debugf("found new network %s (%s)", id, net.netType) } @@ -70,7 +95,7 @@ func (n *networkState) LockingNetwork(iface, ssid string) *lockingNetwork { func (n *networkState) Network(iface, ssid string) network { n.mu.Lock() defer n.mu.Unlock() - id := genNetKey(iface, ssid) + id := n.genNetKey(iface, ssid) ln, ok := n.network[id] if !ok { return network{} diff --git a/subsystems/provisioning/provisioning.go b/subsystems/provisioning/provisioning.go index 0e21218..a40ef5f 100644 --- a/subsystems/provisioning/provisioning.go +++ b/subsystems/provisioning/provisioning.go @@ -162,6 +162,7 @@ func (w *Provisioning) init(ctx context.Context) error { } w.updateHotspotSSID(w.cfg) + w.netState.SetHotspotInterface(w.cfg.HotspotInterface) if err := w.writeDNSMasq(); err != nil { return errw.Wrap(err, "writing dnsmasq configuration") @@ -312,6 +313,7 @@ func (w *Provisioning) Update(ctx context.Context, updateConf *agentpb.DeviceSub if cfg.HotspotInterface == "" { cfg.HotspotInterface = w.Config().HotspotInterface } + w.netState.SetHotspotInterface(cfg.HotspotInterface) if reflect.DeepEqual(cfg, w.cfg) { return needRestart, nil diff --git a/subsystems/provisioning/scanning.go b/subsystems/provisioning/scanning.go index c3eaa8a..f34190a 100644 --- a/subsystems/provisioning/scanning.go +++ b/subsystems/provisioning/scanning.go @@ -169,11 +169,15 @@ func (w *Provisioning) updateKnownConnections(ctx context.Context) error { } ifName, ssid, netType := getIfNameSSIDTypeFromSettings(settings) - if ifName == "" { + if netType == "" { // unknown network type, or broken network continue } + if ifName == "" && netType == NetworkTypeWifi { + ifName = w.Config().HotspotInterface + } + _, ok := highestPriority[ifName] if !ok { highestPriority[ifName] = -999 diff --git a/subsystems/provisioning/setup.go b/subsystems/provisioning/setup.go index 4fef08a..1cc7700 100644 --- a/subsystems/provisioning/setup.go +++ b/subsystems/provisioning/setup.go @@ -124,6 +124,7 @@ func (w *Provisioning) initDevices() error { if w.cfg.HotspotInterface == "" || ifName == w.cfg.HotspotInterface { w.cfg.HotspotInterface = ifName + w.netState.SetHotspotInterface(ifName) w.logger.Infof("Using %s for hotspot/provisioning, will actively manage wifi only on this device.", ifName) } default: