Support nconnect network feature #71

billfort · 2023-07-21T00:51:03Z

nConnect nodes can set up a virtual private network;
nConnect network Manager is a new node role of nConnect, which is the administrator node of the network;
nConnect network Member node can join this Manager's network;
When Manager authorize a member's JoinNetwork request, the member can communicate with other nodes according to the Manager access setting.
Each member node has a network interface that assigned the network's local IP, and members can access each other by this local IP.

yilunzhang · 2023-07-25T00:18:34Z

admin/server.go

@@ -10,6 +10,12 @@ import (
 	tunnel "github.com/nknorg/nkn-tunnel"
 )

+var networkMemberIAccept []string


I don't recommend using global variable

Yes, modified.

yilunzhang · 2023-07-25T00:22:20Z

admin/server.go

 			continue
 		}

 		var perm permission
-		if isAcceptAddr {
+		if isAcceptAddr || isNetworkMemberAddr {


We should separate the permissions of accept addr and member addr

Yes, modified.

yilunzhang · 2023-07-25T00:23:41Z

arch/tun_test.go

+	dnsServers := []string{"192.168.0.1"}
+	persist := false
+
+	OpenTunDevice(name, addr, gw, mask, dnsServers, persist)


This only works on Windows and Mac but not Linux. Check the tun code for different platforms

Yes, got it.

yilunzhang · 2023-07-25T00:31:08Z

config/config.go

@@ -123,6 +125,10 @@ type Config struct {
 	lock        sync.RWMutex
 	AcceptAddrs []string `json:"acceptAddrs"`
 	AdminAddrs  []string `json:"adminAddrs"`
+
+	// nconnect network
+	NodeName       string `json:"nodeName,omitempty" long:"node-name" description:"Node name, will be used as nConnect network node name"`


(network member only) Node name that will be used as to join a network

Yes, modified.

yilunzhang · 2023-07-25T00:34:03Z

config/config.go

+
+	// nconnect network
+	NodeName       string `json:"nodeName,omitempty" long:"node-name" description:"Node name, will be used as nConnect network node name"`
+	ManagerAddress string `json:"managerAddress,omitempty" long:"manager-address" description:"Manager address, will be used as nConnect network manager address"`


(network member only) Manager address to connect to when joining a network

Yes, modified.

yilunzhang · 2023-07-25T06:43:21Z

nconnect.go

-		TunaGeoDBPath:                opts.TunaGeoDBPath,
-		TunaMeasureBandwidth:         !opts.TunaDisableMeasureBandwidth,
-		TunaMeasureStoragePath:       opts.TunaMeasureStoragePath,
-		TunaMeasurementBytesDownLink: opts.TunaMeasureBandwidthBytes,


Why remove this?

I didn't modify this section. Maybe keyboard mistake. let me recover it.

yilunzhang · 2023-07-25T11:01:02Z

nconnect.go

-	err := nc.opts.VerifyClient()
-	if err != nil {
-		return err
+	if !nc.opts.NetworkMember {


I would say we put this if inside VerifyClient()

VerifyClient is a method of Config, and it can't get the mode(client, server, or network) of nConnect. An alter way is to put VerifyClient into Opts.

yilunzhang · 2023-07-25T11:08:35Z

util/util.go

+// If paramenter port is 0, start from system returned free port
+// The returned port is free in TCP and UDP
+func GetFreePort(port int) (int, error) {
+	for i := 0; i < 100; i++ {


retry without sleep is not likely to make a difference. We should add sleep (but also reduce the number of retries so it won't take forever)

Here each retry will increase port. Such as we first retry 1080, if it is busy, then try 1081, 1082, ... until we get a free port. Is it suitable?

Where is the code to increase port? And since you don't need a fixed port, why not a random dynamic port?

when UDP listen fails, the port will be increased to try the next port. if argument port=0, it will try a random dynamic port. This modification expands the use cases of this function: 1. get a free port from system random port; 2 get a specific port or subsequent port, such as 1080, 1081, ...

if err != nil { l.Close() port++ continue }

yilunzhang · 2023-07-25T11:16:05Z

nconnect.go

+		nc.ssClientConfig.Client = from[0]
+		nc.ssClientConfig.DefaultClient = from[0] // the first config is the default client
+	} else {
+		nc.ssClientConfig.Client = "127.0.0.1:0"


This should mean start ss with a random port right? Or you don't want ss at all?

ssClientConfig.Client is just a condition for starting ss client. Before we support multi-tunnels, its value is the from source of the tunnel. Now we support multi-tunnel, the from sources will be gotten from the target IP. so ssClientConfig.Client 's value is only for starting client now. the condition is:
if flags.Client != "" { // client mode

yilunzhang · 2023-07-25T11:18:32Z

nconnect.go


 	log.Println("Client socks proxy listen address:", nc.opts.LocalSocksAddr)

 	if nc.opts.Tun || nc.opts.VPN {
-		tunDevice, err := arch.OpenTunDevice(nc.opts.TunName, nc.opts.TunAddr, nc.opts.TunGateway, nc.opts.TunMask, nc.opts.TunDNS, true)
+		tunDevice, err := arch.OpenTunDevice(nc.opts.TunName, nc.opts.TunAddr, nc.opts.TunGateway, nc.opts.TunMask, nc.opts.TunDNS, false)


FYI persist option only works on Linux. On Mac/Win it's not used

I see. let me restore it.

I mean you can still use it if needed, but you need to make sure the changes will work on all platforms we support

Yes, thanks for the advice.

yilunzhang · 2023-07-25T11:20:56Z

nconnect.go

 	nc.waitForSignal()

 	return nil
 }

-func (nc *nconnect) startSSAndTunnel() {
+func (nc *nconnect) startSSAndTunnel(client bool) {


I would recommend we pass in the config

Yes, modified.

yilunzhang · 2023-07-26T00:44:42Z

nconnect.go

+		}
+		ssAddr := "127.0.0.1:" + strconv.Itoa(port)
+
+		toAddr := strings.Replace(addr, "client.", "", -1) // remove network member's client. prefix, it should be server's address


Do we have to assume the address format here? Why can't we just store and use other members' tunnel address directly?

Yes, modified. The nconnect server's listen address is registered to the network manager and notified to the network authorized members.

yilunzhang · 2023-07-26T00:46:20Z

nconnect.go

+
+		toAddr := strings.Replace(addr, "client.", "", -1) // remove network member's client. prefix, it should be server's address
+		adminAddr := nc.opts.AdminIdentifier + "." + toAddr
+		if remoteInfo, err := nc.getRemoteInfo(adminAddr); err == nil {


For network mode, why we need to use other members' local IP at all? We should use the assigned IP address, which doesn't need getRemoteInfo at all

Yes, modified.

yilunzhang · 2023-07-26T00:51:15Z

nconnect.go

+
+	if len(from) > 0 {
+		identifier := config.RandomIdentifier()
+		tunnels, err := tunnel.NewTunnels(nc.account, identifier, from, to, nc.opts.Tuna, nc.tunnelConfig)


Hmmm, so we are creating a new client each time we update the access address?

Yes, I will modify nkn-tunnel to reuse multi-client.

yilunzhang · 2023-07-26T02:01:18Z

network/manager.go

+
+const (
+	defaultDomain  = "nconnect.nkn"
+	defaultIpStart = "192.168.0.1"


I wouldn't use 192.168 by default, this IP range conflicts with too many routers. I would use 10.X.Y range with a rarely used X and Y

Yes, modified. It is configurable on the manager's web page too.

yilunzhang · 2023-07-26T08:46:07Z

network/message.go

+	return nil, err
+}
+
+func GetMultiClient(opts *config.Opts) (*nkn.MultiClient, error) {


Feels like there are lots of redundant code here

Yes. removed.

yilunzhang · 2023-07-26T08:47:33Z

network/message.go

+	return mc, nil
+}
+
+func GetAdminClient(opts *config.Opts) *admin.Client {


Is there function ever called?

No. Let me remove it.

yilunzhang · 2023-07-26T09:09:00Z

network/message.go

+
+	var onMessage *nkn.OnMessage
+	for i := 0; i < 3; i++ {
+		onMessage, err = mc.Send(nkn.NewStringArray(address), reqBytes, nil)


onMessage -> onReply

Yes, modified.

yilunzhang · 2023-07-26T09:11:24Z

network/message.go

+	NodeInfo    []*nodeInfo  `json:"nodeInfo"`
+}
+
+func SendMsg(mc *admin.Client, address string, msg interface{}, waitResponse bool) (*managerToMember, error) {


Codes in this function seem to be mostly duplicated (admin/client.go:RPCCall())

Yes, modified.

yilunzhang · 2023-07-26T09:13:49Z

network/manager.go

+		if err := m.saveNetworkData(); err != nil {
+			return nil, err
+		}
+		noti := &managerToMember{


what does noti stands for?

Sorry, it is an abbreviation for notification, the message initiated from manager to member is set as notification.
I have modified all noti to notification for better readability.

yilunzhang · 2023-07-26T22:36:20Z

network/manager.go

+	return nil
+}
+
+func (m *Manager) UpdateName(address, name string) error {


Should manager also send message to members on update event? (update name, update ip, etc)

Yes. I added two multicase functions to notify related members.

yilunzhang · 2023-07-26T22:40:03Z

network/manager.go

+
+		for _, n := range m.networkData.Member {
+			acceptAddresses := m.networkData.AcceptAddress[n.Address]
+			if len(acceptAddresses) > 0 && acceptAddresses[0] == AcceptAll {


Anything other than acceptall we should handle?

The manager can set each member's accept nodes. One option is to accept all other members AcceptAll. other options the manager set only some nodes which can accept the designated node.

yilunzhang · 2023-07-27T01:54:28Z

network/manager.go

+
+	networkDataFile = "network.json"
+
+	AcceptAll = "all"


I would suggest we use AllMembers = "allMembers". There might be case when we have public nodes that accept connections from everyone (even not a member).

Yes, modified.

yilunzhang · 2023-07-27T04:30:56Z

network/manager.go

+		return errNameExist
+	}
+
+	var ok1, ok2 bool


ok1 is defined twice.
Also I would recommend using isMember and isWaiting

Yes, modified.

Signed-off-by: bill fort <[email protected]>

billfort force-pushed the network branch from 8bcc906 to e618ca6 Compare July 21, 2023 16:27

yilunzhang reviewed Jul 25, 2023

View reviewed changes

yilunzhang reviewed Jul 26, 2023

View reviewed changes

yilunzhang reviewed Jul 27, 2023

View reviewed changes

billfort force-pushed the network branch 4 times, most recently from ea3932e to 07a0f9c Compare August 11, 2023 20:29

billfort force-pushed the network branch from 07a0f9c to 854d40d Compare August 15, 2023 21:30

billfort force-pushed the network branch 6 times, most recently from 8e4b31f to 779a4a9 Compare August 29, 2023 16:57

billfort force-pushed the network branch 9 times, most recently from b329261 to 579e873 Compare September 7, 2023 02:38

billfort force-pushed the network branch 2 times, most recently from 61ead41 to bb677e4 Compare September 13, 2023 01:44

billfort force-pushed the network branch from bb677e4 to abb6437 Compare November 21, 2023 06:22

Support nconnect network feature

6d7e180

Signed-off-by: bill fort <[email protected]>

billfort force-pushed the network branch from abb6437 to 6d7e180 Compare November 22, 2023 07:10

yilunzhang approved these changes Nov 24, 2023

View reviewed changes

yilunzhang merged commit eecf151 into nknorg:master Nov 24, 2023
1 check passed

Support nconnect network feature #71

Support nconnect network feature #71

Conversation

billfort commented Jul 21, 2023

Choose a reason for hiding this comment

billfort Aug 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yilunzhang Jul 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yilunzhang Jul 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

billfort Jul 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

billfort Aug 1, 2023 •

edited

Loading

yilunzhang Jul 25, 2023 •

edited

Loading

yilunzhang Jul 26, 2023 •

edited

Loading

billfort Jul 26, 2023 •

edited

Loading