From fc9a1c6c31b456fb55dd7b5c555975652573b44c Mon Sep 17 00:00:00 2001 From: Neil Alexander Date: Wed, 28 Aug 2019 19:31:04 +0100 Subject: [PATCH] Simplify reconfiguration --- cmd/yggdrasil/main.go | 1 + src/multicast/multicast.go | 1 - src/tuntap/ckr.go | 66 +++++++++++++++----------------------- src/tuntap/tun.go | 25 +++------------ src/yggdrasil/core.go | 44 ++++--------------------- src/yggdrasil/dht.go | 3 +- src/yggdrasil/link.go | 9 ++---- src/yggdrasil/peer.go | 3 +- src/yggdrasil/router.go | 20 ++++++------ src/yggdrasil/search.go | 3 +- src/yggdrasil/session.go | 17 +++------- src/yggdrasil/switch.go | 5 +-- src/yggdrasil/tcp.go | 8 +++-- 13 files changed, 64 insertions(+), 141 deletions(-) diff --git a/cmd/yggdrasil/main.go b/cmd/yggdrasil/main.go index 2fa10720..e9a21c13 100644 --- a/cmd/yggdrasil/main.go +++ b/cmd/yggdrasil/main.go @@ -279,6 +279,7 @@ func main() { case _ = <-r: if *useconffile != "" { cfg = readConfig(useconf, useconffile, normaliseconf) + logger.Infoln("Reloading configuration from", *useconffile) n.core.UpdateConfig(cfg) n.tuntap.UpdateConfig(cfg) n.multicast.UpdateConfig(cfg) diff --git a/src/multicast/multicast.go b/src/multicast/multicast.go index 5ab9673e..22ac9356 100644 --- a/src/multicast/multicast.go +++ b/src/multicast/multicast.go @@ -83,7 +83,6 @@ func (m *Multicast) Stop() error { func (m *Multicast) UpdateConfig(config *config.NodeConfig) { m.log.Debugln("Reloading multicast configuration...") m.config.Replace(*config) - m.log.Infoln("Multicast configuration reloaded successfully") } // GetInterfaces returns the currently known/enabled multicast interfaces. It is diff --git a/src/tuntap/ckr.go b/src/tuntap/ckr.go index 229d2607..9af3564a 100644 --- a/src/tuntap/ckr.go +++ b/src/tuntap/ckr.go @@ -20,7 +20,6 @@ import ( type cryptokey struct { tun *TunAdapter enabled atomic.Value // bool - reconfigure chan chan error ipv4remotes []cryptokey_route ipv6remotes []cryptokey_route ipv4cache map[address.Address]cryptokey_route @@ -40,24 +39,11 @@ type cryptokey_route struct { // Initialise crypto-key routing. This must be done before any other CKR calls. func (c *cryptokey) init(tun *TunAdapter) { c.tun = tun - c.reconfigure = make(chan chan error, 1) - go func() { - for { - e := <-c.reconfigure - e <- nil - } - }() - - c.tun.log.Debugln("Configuring CKR...") - if err := c.configure(); err != nil { - c.tun.log.Errorln("CKR configuration failed:", err) - } else { - c.tun.log.Debugln("CKR configured") - } + c.configure() } -// Configure the CKR routes. -func (c *cryptokey) configure() error { +// Configure the CKR routes. This should only ever be ran by the TUN/TAP actor. +func (c *cryptokey) configure() { current := c.tun.config.GetCurrent() // Set enabled/disabled state @@ -72,14 +58,14 @@ func (c *cryptokey) configure() error { // Add IPv6 routes for ipv6, pubkey := range current.TunnelRouting.IPv6RemoteSubnets { if err := c.addRemoteSubnet(ipv6, pubkey); err != nil { - return err + c.tun.log.Errorln("Error adding CKR IPv6 remote subnet:", err) } } // Add IPv4 routes for ipv4, pubkey := range current.TunnelRouting.IPv4RemoteSubnets { if err := c.addRemoteSubnet(ipv4, pubkey); err != nil { - return err + c.tun.log.Errorln("Error adding CKR IPv4 remote subnet:", err) } } @@ -93,7 +79,7 @@ func (c *cryptokey) configure() error { c.ipv6locals = make([]net.IPNet, 0) for _, source := range current.TunnelRouting.IPv6LocalSubnets { if err := c.addLocalSubnet(source); err != nil { - return err + c.tun.log.Errorln("Error adding CKR IPv6 local subnet:", err) } } @@ -101,7 +87,7 @@ func (c *cryptokey) configure() error { c.ipv4locals = make([]net.IPNet, 0) for _, source := range current.TunnelRouting.IPv4LocalSubnets { if err := c.addLocalSubnet(source); err != nil { - return err + c.tun.log.Errorln("Error adding CKR IPv4 local subnet:", err) } } @@ -110,8 +96,6 @@ func (c *cryptokey) configure() error { c.ipv4cache = make(map[address.Address]cryptokey_route, 0) c.ipv6cache = make(map[address.Address]cryptokey_route, 0) c.mutexcaches.Unlock() - - return nil } // Enable or disable crypto-key routing. @@ -181,19 +165,19 @@ func (c *cryptokey) addLocalSubnet(cidr string) error { } else if prefixsize == net.IPv4len*8 { routingsources = &c.ipv4locals } else { - return errors.New("Unexpected prefix size") + return errors.New("unexpected prefix size") } // Check if we already have this CIDR for _, subnet := range *routingsources { if subnet.String() == ipnet.String() { - return errors.New("Source subnet already configured") + return errors.New("local subnet already configured") } } // Add the source subnet *routingsources = append(*routingsources, *ipnet) - c.tun.log.Infoln("Added CKR source subnet", cidr) + c.tun.log.Infoln("Added CKR local subnet", cidr) return nil } @@ -226,7 +210,7 @@ func (c *cryptokey) addRemoteSubnet(cidr string, dest string) error { routingtable = &c.ipv4remotes routingcache = &c.ipv4cache } else { - return errors.New("Unexpected prefix size") + return errors.New("unexpected prefix size") } // Is the route an Yggdrasil destination? @@ -235,19 +219,19 @@ func (c *cryptokey) addRemoteSubnet(cidr string, dest string) error { copy(addr[:], ipaddr) copy(snet[:], ipnet.IP) if addr.IsValid() || snet.IsValid() { - return errors.New("Can't specify Yggdrasil destination as crypto-key route") + return errors.New("can't specify Yggdrasil destination as crypto-key route") } // Do we already have a route for this subnet? for _, route := range *routingtable { if route.subnet.String() == ipnet.String() { - return errors.New(fmt.Sprintf("Route already exists for %s", cidr)) + return fmt.Errorf("remote subnet already exists for %s", cidr) } } // Decode the public key if bpk, err := hex.DecodeString(dest); err != nil { return err } else if len(bpk) != crypto.BoxPubKeyLen { - return errors.New(fmt.Sprintf("Incorrect key length for %s", dest)) + return fmt.Errorf("incorrect key length for %s", dest) } else { // Add the new crypto-key route var key crypto.BoxPubKey @@ -270,7 +254,7 @@ func (c *cryptokey) addRemoteSubnet(cidr string, dest string) error { delete(*routingcache, k) } - c.tun.log.Infoln("Added CKR destination subnet", cidr) + c.tun.log.Infoln("Added CKR remote subnet", cidr) return nil } } @@ -284,7 +268,7 @@ func (c *cryptokey) getPublicKeyForAddress(addr address.Address, addrlen int) (c // Check if the address is a valid Yggdrasil address - if so it // is exempt from all CKR checking if addr.IsValid() { - return crypto.BoxPubKey{}, errors.New("Cannot look up CKR for Yggdrasil addresses") + return crypto.BoxPubKey{}, errors.New("cannot look up CKR for Yggdrasil addresses") } // Build our references to the routing table and cache @@ -297,7 +281,7 @@ func (c *cryptokey) getPublicKeyForAddress(addr address.Address, addrlen int) (c } else if addrlen == net.IPv4len { routingcache = &c.ipv4cache } else { - return crypto.BoxPubKey{}, errors.New("Unexpected prefix size") + return crypto.BoxPubKey{}, errors.New("unexpected prefix size") } // Check if there's a cache entry for this addr @@ -317,7 +301,7 @@ func (c *cryptokey) getPublicKeyForAddress(addr address.Address, addrlen int) (c } else if addrlen == net.IPv4len { routingtable = &c.ipv4remotes } else { - return crypto.BoxPubKey{}, errors.New("Unexpected prefix size") + return crypto.BoxPubKey{}, errors.New("unexpected prefix size") } // No cache was found - start by converting the address into a net.IP @@ -378,18 +362,18 @@ func (c *cryptokey) removeLocalSubnet(cidr string) error { } else if prefixsize == net.IPv4len*8 { routingsources = &c.ipv4locals } else { - return errors.New("Unexpected prefix size") + return errors.New("unexpected prefix size") } // Check if we already have this CIDR for idx, subnet := range *routingsources { if subnet.String() == ipnet.String() { *routingsources = append((*routingsources)[:idx], (*routingsources)[idx+1:]...) - c.tun.log.Infoln("Removed CKR source subnet", cidr) + c.tun.log.Infoln("Removed CKR local subnet", cidr) return nil } } - return errors.New("Source subnet not found") + return errors.New("local subnet not found") } // Removes a destination route for the given CIDR to be tunnelled to the node @@ -421,7 +405,7 @@ func (c *cryptokey) removeRemoteSubnet(cidr string, dest string) error { routingtable = &c.ipv4remotes routingcache = &c.ipv4cache } else { - return errors.New("Unexpected prefix size") + return errors.New("unexpected prefix size") } // Decode the public key @@ -429,7 +413,7 @@ func (c *cryptokey) removeRemoteSubnet(cidr string, dest string) error { if err != nil { return err } else if len(bpk) != crypto.BoxPubKeyLen { - return errors.New(fmt.Sprintf("Incorrect key length for %s", dest)) + return fmt.Errorf("incorrect key length for %s", dest) } netStr := ipnet.String() @@ -439,9 +423,9 @@ func (c *cryptokey) removeRemoteSubnet(cidr string, dest string) error { for k := range *routingcache { delete(*routingcache, k) } - c.tun.log.Infof("Removed CKR destination subnet %s via %s\n", cidr, dest) + c.tun.log.Infof("Removed CKR remote subnet %s via %s\n", cidr, dest) return nil } } - return errors.New(fmt.Sprintf("Route does not exists for %s", cidr)) + return fmt.Errorf("route does not exists for %s", cidr) } diff --git a/src/tuntap/tun.go b/src/tuntap/tun.go index 6317459c..8e1e5b0c 100644 --- a/src/tuntap/tun.go +++ b/src/tuntap/tun.go @@ -13,6 +13,7 @@ import ( "errors" "fmt" "net" + //"sync" "github.com/Arceliar/phony" @@ -200,29 +201,11 @@ func (tun *TunAdapter) _stop() error { func (tun *TunAdapter) UpdateConfig(config *config.NodeConfig) { tun.log.Debugln("Reloading TUN/TAP configuration...") + // Replace the active configuration with the supplied one tun.config.Replace(*config) - errors := 0 - - components := []chan chan error{ - tun.reconfigure, - tun.ckr.reconfigure, - } - - for _, component := range components { - response := make(chan error) - component <- response - if err := <-response; err != nil { - tun.log.Errorln(err) - errors++ - } - } - - if errors > 0 { - tun.log.Warnln(errors, "TUN/TAP module(s) reported errors during configuration reload") - } else { - tun.log.Infoln("TUN/TAP configuration reloaded successfully") - } + // Notify children about the configuration change + tun.Act(nil, tun.ckr.configure) } func (tun *TunAdapter) handler() error { diff --git a/src/yggdrasil/core.go b/src/yggdrasil/core.go index d48b6479..831109dd 100644 --- a/src/yggdrasil/core.go +++ b/src/yggdrasil/core.go @@ -20,6 +20,7 @@ type Core struct { // This is the main data structure that holds everything else for a node // We're going to keep our own copy of the provided config - that way we can // guarantee that it will be covered by the mutex + phony.Inbox config config.NodeState // Config boxPub crypto.BoxPubKey boxPriv crypto.BoxPrivKey @@ -112,47 +113,14 @@ func (c *Core) addPeerLoop() { // config.NodeConfig and then signals the various module goroutines to // reconfigure themselves if needed. func (c *Core) UpdateConfig(config *config.NodeConfig) { - c.log.Infoln("Reloading node configuration...") + c.log.Debugln("Reloading node configuration...") + // Replace the active configuration with the supplied one c.config.Replace(*config) - errors := 0 - // Each reconfigure function should pass any errors to the channel, then close it - components := map[phony.Actor][]func(chan error){ - &c.router: []func(chan error){ - c.router.reconfigure, - c.router.dht.reconfigure, - c.router.searches.reconfigure, - c.router.sessions.reconfigure, - }, - &c.switchTable: []func(chan error){ - c.switchTable.reconfigure, - c.link.reconfigure, - c.peers.reconfigure, - }, - } - - // TODO: We count errors here but honestly that provides us with absolutely no - // benefit over components reporting errors themselves, so maybe we can use - // actor.Act() here instead and stop counting errors - for actor, functions := range components { - for _, function := range functions { - response := make(chan error) - phony.Block(actor, func() { - function(response) - }) - for err := range response { - c.log.Errorln(err) - errors++ - } - } - } - - if errors > 0 { - c.log.Warnln(errors, "node module(s) reported errors during configuration reload") - } else { - c.log.Infoln("Node configuration reloaded successfully") - } + // Notify the router and switch about the new configuration + c.router.Act(c, c.router.reconfigure) + c.switchTable.Act(c, c.switchTable.reconfigure) } // Start starts up Yggdrasil using the provided config.NodeConfig, and outputs diff --git a/src/yggdrasil/dht.go b/src/yggdrasil/dht.go index 4f380363..575c8b1a 100644 --- a/src/yggdrasil/dht.go +++ b/src/yggdrasil/dht.go @@ -82,8 +82,7 @@ func (t *dht) init(r *router) { t.reset() } -func (t *dht) reconfigure(e chan error) { - defer close(e) +func (t *dht) reconfigure() { // This is where reconfiguration would go, if we had anything to do } diff --git a/src/yggdrasil/link.go b/src/yggdrasil/link.go index ee4b9815..6e393514 100644 --- a/src/yggdrasil/link.go +++ b/src/yggdrasil/link.go @@ -79,13 +79,8 @@ func (l *link) init(c *Core) error { return nil } -func (l *link) reconfigure(e chan error) { - defer close(e) - tcpResponse := make(chan error) - l.tcp.reconfigure(tcpResponse) - for err := range tcpResponse { - e <- err - } +func (l *link) reconfigure() { + l.tcp.reconfigure() } func (l *link) call(uri string, sintf string) error { diff --git a/src/yggdrasil/peer.go b/src/yggdrasil/peer.go index 8bd638c4..381e6917 100644 --- a/src/yggdrasil/peer.go +++ b/src/yggdrasil/peer.go @@ -34,8 +34,7 @@ func (ps *peers) init(c *Core) { ps.core = c } -func (ps *peers) reconfigure(e chan error) { - defer close(e) +func (ps *peers) reconfigure() { // This is where reconfiguration would go, if we had anything to do } diff --git a/src/yggdrasil/router.go b/src/yggdrasil/router.go index 5f894d9b..64c81701 100644 --- a/src/yggdrasil/router.go +++ b/src/yggdrasil/router.go @@ -73,18 +73,20 @@ func (r *router) init(core *Core) { r.sessions.init(r) } -func (r *router) reconfigure(e chan error) { - defer close(e) - var errs []error +// Reconfigures the router and any child modules. This should only ever be run +// by the router actor. +func (r *router) reconfigure() { // Reconfigure the router current := r.core.config.GetCurrent() - err := r.nodeinfo.setNodeInfo(current.NodeInfo, current.NodeInfoPrivacy) - if err != nil { - errs = append(errs, err) - } - for _, err := range errs { - e <- err + if err := r.nodeinfo.setNodeInfo(current.NodeInfo, current.NodeInfoPrivacy); err != nil { + r.core.log.Errorln("Error reloading NodeInfo:", err) + } else { + r.core.log.Infoln("NodeInfo updated") } + // Reconfigure children + r.dht.reconfigure() + r.searches.reconfigure() + r.sessions.reconfigure() } // Starts the tickerLoop goroutine. diff --git a/src/yggdrasil/search.go b/src/yggdrasil/search.go index ca357cc2..c128175b 100644 --- a/src/yggdrasil/search.go +++ b/src/yggdrasil/search.go @@ -55,8 +55,7 @@ func (s *searches) init(r *router) { s.searches = make(map[crypto.NodeID]*searchInfo) } -func (s *searches) reconfigure(e chan error) { - defer close(e) +func (s *searches) reconfigure() { // This is where reconfiguration would go, if we had anything to do } diff --git a/src/yggdrasil/session.go b/src/yggdrasil/session.go index f6855b4d..0b55aac8 100644 --- a/src/yggdrasil/session.go +++ b/src/yggdrasil/session.go @@ -73,8 +73,7 @@ type sessionInfo struct { callbacks []chan func() // Finished work from crypto workers } -func (sinfo *sessionInfo) reconfigure(e chan error) { - defer close(e) +func (sinfo *sessionInfo) reconfigure() { // This is where reconfiguration would go, if we had anything to do } @@ -161,17 +160,9 @@ func (ss *sessions) init(r *router) { ss.lastCleanup = time.Now() } -func (ss *sessions) reconfigure(e chan error) { - defer close(e) - responses := make(map[crypto.Handle]chan error) - for index, session := range ss.sinfos { - responses[index] = make(chan error) - session.reconfigure(responses[index]) - } - for _, response := range responses { - for err := range response { - e <- err - } +func (ss *sessions) reconfigure() { + for _, session := range ss.sinfos { + session.reconfigure() } } diff --git a/src/yggdrasil/switch.go b/src/yggdrasil/switch.go index 5c613d8d..b6bd5b96 100644 --- a/src/yggdrasil/switch.go +++ b/src/yggdrasil/switch.go @@ -199,9 +199,10 @@ func (t *switchTable) init(core *Core) { }) } -func (t *switchTable) reconfigure(e chan error) { - defer close(e) +func (t *switchTable) reconfigure() { // This is where reconfiguration would go, if we had anything useful to do. + t.core.link.reconfigure() + t.core.peers.reconfigure() } // Safely gets a copy of this node's locator. diff --git a/src/yggdrasil/tcp.go b/src/yggdrasil/tcp.go index ccb488f9..cce352bd 100644 --- a/src/yggdrasil/tcp.go +++ b/src/yggdrasil/tcp.go @@ -95,8 +95,7 @@ func (t *tcp) init(l *link) error { return nil } -func (t *tcp) reconfigure(e chan error) { - defer close(e) +func (t *tcp) reconfigure() { t.link.core.config.Mutex.RLock() added := util.Difference(t.link.core.config.Current.Listen, t.link.core.config.Previous.Listen) deleted := util.Difference(t.link.core.config.Previous.Listen, t.link.core.config.Current.Listen) @@ -107,7 +106,9 @@ func (t *tcp) reconfigure(e chan error) { continue } if _, err := t.listen(a[6:]); err != nil { - e <- err + t.link.core.log.Errorln("Error adding TCP", a[6:], "listener:", err) + } else { + t.link.core.log.Infoln("Started TCP listener:", a[6:]) } } for _, d := range deleted { @@ -118,6 +119,7 @@ func (t *tcp) reconfigure(e chan error) { if listener, ok := t.listeners[d[6:]]; ok { t.mutex.Unlock() listener.Stop <- true + t.link.core.log.Infoln("Stopped TCP listener:", d[6:]) } else { t.mutex.Unlock() }