Reimplement AddPeer and RemovePeer for admin socket (#951)

* Reimplement AddPeer and RemovePeer for admin socket

Fix #950

* Disconnect the peer on `removePeer`

Co-authored-by: Neil Alexander <neilalexander@users.noreply.github.com>

src/admin/addpeer.go

@@ -0,0 +1,12 @@
+package admin
+
+type AddPeerRequest struct {
+	Uri   string `json:"uri"`
+	Sintf string `json:"interface,omitempty"`
+}
+
+type AddPeerResponse struct{}
+
+func (a *AdminSocket) addPeerHandler(req *AddPeerRequest, res *AddPeerResponse) error {
+	return a.core.AddPeer(req.Uri, req.Sintf)
+}

src/admin/admin.go

@@ -174,6 +174,34 @@ func (a *AdminSocket) SetupAdminHandlers() {
 			return res, nil
 		},
 	)
+	_ = a.AddHandler(
+		"addPeer", "Add a peer to the peer list", []string{"uri", "interface"},
+		func(in json.RawMessage) (interface{}, error) {
+			req := &AddPeerRequest{}
+			res := &AddPeerResponse{}
+			if err := json.Unmarshal(in, &req); err != nil {
+				return nil, err
+			}
+			if err := a.addPeerHandler(req, res); err != nil {
+				return nil, err
+			}
+			return res, nil
+		},
+	)
+	_ = a.AddHandler(
+		"removePeer", "Remove a peer from the peer list", []string{"uri", "interface"},
+		func(in json.RawMessage) (interface{}, error) {
+			req := &RemovePeerRequest{}
+			res := &RemovePeerResponse{}
+			if err := json.Unmarshal(in, &req); err != nil {
+				return nil, err
+			}
+			if err := a.removePeerHandler(req, res); err != nil {
+				return nil, err
+			}
+			return res, nil
+		},
+	)
 	//_ = a.AddHandler("getNodeInfo", []string{"key"}, t.proto.nodeinfo.nodeInfoAdminHandler)
 	//_ = a.AddHandler("debug_remoteGetSelf", []string{"key"}, t.proto.getSelfHandler)
 	//_ = a.AddHandler("debug_remoteGetPeers", []string{"key"}, t.proto.getPeersHandler)

src/admin/removepeer.go

@@ -0,0 +1,12 @@
+package admin
+
+type RemovePeerRequest struct {
+	Uri   string `json:"uri"`
+	Sintf string `json:"interface,omitempty"`
+}
+
+type RemovePeerResponse struct{}
+
+func (a *AdminSocket) removePeerHandler(req *RemovePeerRequest, res *RemovePeerResponse) error {
+	return a.core.RemovePeer(req.Uri, req.Sintf)
+}

src/core/api.go

@@ -181,78 +181,49 @@ func (c *Core) SetLogger(log util.Logger) {
 }
 
 // AddPeer adds a peer. This should be specified in the peer URI format, e.g.:
-// 		tcp://a.b.c.d:e
-//		socks://a.b.c.d:e/f.g.h.i:j
+//
+//	tcp://a.b.c.d:e
+//	socks://a.b.c.d:e/f.g.h.i:j
+//
 // This adds the peer to the peer list, so that they will be called again if the
 // connection drops.
-/*
-func (c *Core) AddPeer(addr string, sintf string) error {
-	if err := c.CallPeer(addr, sintf); err != nil {
-		// TODO: We maybe want this to write the peer to the persistent
-		// configuration even if a connection attempt fails, but first we'll need to
-		// move the code to check the peer URI so that we don't deliberately save a
-		// peer with a known bad URI. Loading peers from config should really do the
-		// same thing too but I don't think that happens today
+func (c *Core) AddPeer(uri string, sourceInterface string) error {
+	u, err := url.Parse(uri)
+	if err != nil {
 		return err
 	}
-	c.config.Mutex.Lock()
-	defer c.config.Mutex.Unlock()
-	if sintf == "" {
-		for _, peer := range c.config.Current.Peers {
-			if peer == addr {
-				return errors.New("peer already added")
-			}
-		}
-		c.config.Current.Peers = append(c.config.Current.Peers, addr)
-	} else {
-		if _, ok := c.config.Current.InterfacePeers[sintf]; ok {
-			for _, peer := range c.config.Current.InterfacePeers[sintf] {
-				if peer == addr {
-					return errors.New("peer already added")
-				}
-			}
-		}
-		if _, ok := c.config.Current.InterfacePeers[sintf]; !ok {
-			c.config.Current.InterfacePeers[sintf] = []string{addr}
-		} else {
-			c.config.Current.InterfacePeers[sintf] = append(c.config.Current.InterfacePeers[sintf], addr)
-		}
+	info, err := c.links.call(u, sourceInterface)
+	if err != nil {
+		return err
 	}
-	return nil
-}
-*/
-
-/*
-func (c *Core) RemovePeer(addr string, sintf string) error {
-	if sintf == "" {
-		for i, peer := range c.config.Current.Peers {
-			if peer == addr {
-				c.config.Current.Peers = append(c.config.Current.Peers[:i], c.config.Current.Peers[i+1:]...)
-				break
-			}
-		}
-	} else if _, ok := c.config.Current.InterfacePeers[sintf]; ok {
-		for i, peer := range c.config.Current.InterfacePeers[sintf] {
-			if peer == addr {
-				c.config.Current.InterfacePeers[sintf] = append(c.config.Current.InterfacePeers[sintf][:i], c.config.Current.InterfacePeers[sintf][i+1:]...)
-				break
-			}
-		}
-	}
-
-	panic("TODO") // Get the net.Conn to this peer (if any) and close it
-	c.peers.Act(nil, func() {
-		ports := c.peers.ports
-		for _, peer := range ports {
-			if addr == peer.intf.name() {
-				c.peers._removePeer(peer)
-			}
-		}
+	phony.Block(c, func() {
+		c.config._peers[Peer{uri, sourceInterface}] = &info
 	})
-
 	return nil
 }
-*/
+
+// RemovePeer removes a peer. The peer should be specified in URI format, see AddPeer.
+// The peer is not disconnected immediately.
+func (c *Core) RemovePeer(uri string, sourceInterface string) error {
+	var err error
+	phony.Block(c, func() {
+		peer := Peer{uri, sourceInterface}
+		linkInfo, ok := c.config._peers[peer]
+		if !ok {
+			err = fmt.Errorf("peer not configured")
+			return
+		}
+		if ok && linkInfo != nil {
+			c.links.Act(nil, func() {
+				if link := c.links._links[*linkInfo]; link != nil {
+					_ = link.close()
+				}
+			})
+		}
+		delete(c.config._peers, peer)
+	})
+	return err
+}
 
 // CallPeer calls a peer once. This should be specified in the peer URI format,
 // e.g.:
@@ -263,7 +234,8 @@ func (c *Core) RemovePeer(addr string, sintf string) error {
 // This does not add the peer to the peer list, so if the connection drops, the
 // peer will not be called again automatically.
 func (c *Core) CallPeer(u *url.URL, sintf string) error {
-	return c.links.call(u, sintf)
+	_, err := c.links.call(u, sintf)
+	return err
 }
 
 func (c *Core) PublicKey() ed25519.PublicKey {

src/core/core.go

@@ -36,7 +36,7 @@ type Core struct {
 	log          util.Logger
 	addPeerTimer *time.Timer
 	config       struct {
-		_peers             map[Peer]struct{}          // configurable after startup
+		_peers             map[Peer]*linkInfo         // configurable after startup
 		_listeners         map[ListenAddress]struct{} // configurable after startup
 		nodeinfo           NodeInfo                   // immutable after startup
 		nodeinfoPrivacy    NodeInfoPrivacy            // immutable after startup
@@ -66,7 +66,7 @@ func New(secret ed25519.PrivateKey, logger util.Logger, opts ...SetupOption) (*C
 	if c.PacketConn, err = iwe.NewPacketConn(c.secret); err != nil {
 		return nil, fmt.Errorf("error creating encryption: %w", err)
 	}
-	c.config._peers = map[Peer]struct{}{}
+	c.config._peers = map[Peer]*linkInfo{}
 	c.config._listeners = map[ListenAddress]struct{}{}
 	c.config._allowedPublicKeys = map[[32]byte]struct{}{}
 	for _, opt := range opts {

src/core/link.go

@@ -108,10 +108,10 @@ func (l *links) isConnectedTo(info linkInfo) bool {
 	return isConnected
 }
 
-func (l *links) call(u *url.URL, sintf string) error {
+func (l *links) call(u *url.URL, sintf string) (linkInfo, error) {
 	info := linkInfoFor(u.Scheme, sintf, u.Host)
 	if l.isConnectedTo(info) {
-		return nil
+		return info, nil
 	}
 	options := linkOptions{
 		pinnedEd25519Keys: map[keyArray]struct{}{},
@@ -119,7 +119,7 @@ func (l *links) call(u *url.URL, sintf string) error {
 	for _, pubkey := range u.Query()["key"] {
 		sigPub, err := hex.DecodeString(pubkey)
 		if err != nil {
-			return fmt.Errorf("pinned key contains invalid hex characters")
+			return info, fmt.Errorf("pinned key contains invalid hex characters")
 		}
 		var sigPubKey keyArray
 		copy(sigPubKey[:], sigPub)
@@ -172,9 +172,9 @@ func (l *links) call(u *url.URL, sintf string) error {
 		}()
 
 	default:
-		return errors.New("unknown call scheme: " + u.Scheme)
+		return info, errors.New("unknown call scheme: " + u.Scheme)
 	}
-	return nil
+	return info, nil
 }
 
 func (l *links) listen(u *url.URL, sintf string) (*Listener, error) {

src/core/options.go

@@ -7,7 +7,7 @@ import (
 func (c *Core) _applyOption(opt SetupOption) {
 	switch v := opt.(type) {
 	case Peer:
-		c.config._peers[v] = struct{}{}
+		c.config._peers[v] = nil
 	case ListenAddress:
 		c.config._listeners[v] = struct{}{}
 	case NodeInfo: