Refactor proxies to use the same UTP socket for external connections

[tegefaulkes]

Specification

There is a problem with obtaining the ReverseConnection's NATified port during the NAT traversial process. If we can combine the two sockets that the ForwardProxy and the ReverseProxy uses we can greatly simplify the problem. The result being that if we receive a connection from a node, to establish a connection back we can use the same IP andPort` information.

The ReverseProxy and ForwardProxy needs to share a UTP socket. To this end we need to combine the ForwardProxy and ReverseProxy into a single Proxy class. any config parameters will need to be updated. duplicated logic between the two proxies will need to be combined together. anything specific to the forward or reverse case will need to remain separate. Things like openConnection will need to be differentiated by forwardOpenConnection reverseOpenConnection ect.

The old proxies will need to be removed and any tests using them will need to be updated to use the new Proxy class.

New naming of all ports and hosts:

┌────────────┐ clientHost forwardHost                               forwardHost clientHost┌────────────┐
│            │ clientPort forwardPort                               forwardPort clientPort│            │
│ GRPCClient ├────────────────┐                                            ┌──────────────┤ GRPCClient │
│            │                │      ┌───────┐             ┌───────┐       │              │            │
└────────────┘                └──────►       │             │       ◄───────┘              └────────────┘
                                     │ Proxy ◄─────────────► Proxy │
┌────────────┐                ┌──────┤       │  proxyHost  │       ├───────┐              ┌────────────┐
│            │                │      └───────┘  proxyPort  └───────┘       │              │            │
│ GRPCServer ◄────────────────┘                                            └──────────────► GRPCServer │
│            │ serverHost reverseHost                               reverseHost serverHost│            │
└────────────┘ serverPort reversePort                               reversePort serverPort└────────────┘

Additional context

• Reference https://github.com/MatrixAI/js-polykey/pull/326#issuecomment-1071958157
• Reference https://github.com/MatrixAI/js-polykey/issues/344#issuecomment-1071914996

Tasks

• [x] 1. prototype using a single UTP socket shared between the ForwardProxy and ReverseProxy.
• [x] 2. combine ForwardProxy and ReverseProxy into a single Proxy class.
• [x] 3. update config types and config defaults to reflect egress and ingress being the same now.
• [x] 4. remove old ReverseProxy and ForwardProxy modules.
• [x] 5. update any code that uses the old proxies to use the new Proxy module
• [x] 6. update any tests to use the new Proxy module
• [x] 7. combine fwd/rev proxy tests into Proxy.test.ts
• [x] 8. update host/port names.

[tegefaulkes]

I need working code so I'm going to branch off of master for this. Work for this should be mostly within networking domain so there should be minimal conflicts with the testnetprep branch.

[tegefaulkes]

@CMCDragonkai has confirmed that we can share the socket between the two proxies. Here is an example of the code used for testing this.

// AGENT 1

import UTP from 'utp-native';
import { promisify } from './src/utils';

async function main () {

  const utpSocket = new UTP({
    allowHalfOpen: true,
  });

  const utpSocketListen = promisify(utpSocket.listen).bind(utpSocket);
  await utpSocketListen(55555, '0.0.0.0');

  utpSocket.on('connection', (utpConn) => {
    console.log(`Received connection on ${utpConn.remoteAddress}:${utpConn.remotePort}`);
    utpConn.pipe(utpConn);
  });

  const utpConn = utpSocket.connect(55556, '127.0.0.1', { allowHalfOpen: true });
  utpConn.write('Hello from agent1');
  utpConn.end();
  utpConn.on('data', (data) => {
    console.log('DATA', data.toString());
  });
  utpConn.on('end', () => {
    console.log('Ended connection');
  });

  console.log('STARTED agent1');
}

main();

// AGENT 2

import UTP from 'utp-native';
import { promisify } from './src/utils';

async function main () {

  const utpSocket = new UTP({
    allowHalfOpen: true,
  });

  const utpSocketListen = promisify(utpSocket.listen).bind(utpSocket);
  await utpSocketListen(55556, '0.0.0.0');

  utpSocket.on('connection', (utpConn) => {
    console.log(`Received connection on ${utpConn.remoteAddress}:${utpConn.remotePort}`);
    utpConn.pipe(utpConn);
  });

  const utpConn = utpSocket.connect(55555, '127.0.0.1', { allowHalfOpen: true });
  utpConn.write('Hello from agent2');
  utpConn.end();
  utpConn.on('data', (data) => {
    console.log('DATA', data.toString());
  });
  utpConn.on('end', () => {
    console.log('Ended connection');
  });
  console.log('STARTED agent2');
}

main();

[nix-shell:~/Projects/js-polykey]$ npm run ts-node -- ./agent1.ts 

> @matrixai/polykey@1.0.0 ts-node /home/cmcdragonkai/Projects/js-polykey
> ts-node --require tsconfig-paths/register "./agent1.ts"

STARTED agent1
Received connection on 127.0.0.1:55556
DATA Hello from agent1
Ended connection

[nix-shell:~/Projects/js-polykey]$ npm run ts-node -- ./agent2.ts 
> @matrixai/polykey@1.0.0 ts-node /home/cmcdragonkai/Projects/js-polykey
> ts-node --require tsconfig-paths/register "./agent2.ts"

STARTED agent2
DATA Hello from agent2
Ended connection
Received connection on 127.0.0.1:55555

[tegefaulkes]

Cool. I've prototyped some changes and It is working in test cases such as

• starting a PolykeyAgent
• running CLI pk agent start to start in foreground.
• doing an agent-agent GRPC connection. Cross signing claims in this case.

One more test to check is concurrently doing a agent-agent call from both sides. such as getting the closest local nodes from both agents at the same time. We want to test a connection in both directions at the same time. I expect this to be working but a test for it should be done.

From here I can

• spec out the issue propely
• clean up the prototyping
• combine the ForwardProxy and ReverseProxy into a single Proxy class
• fix up configs and defaults
• fix up tests

[CMCDragonkai]

This will need an update to the Network wiki page too: https://github.com/MatrixAI/js-polykey/wiki/network

[tegefaulkes]

Finished combining the rev and fwd proxies into Proxy. I've run some spot tests to check that it is working and they are running fine.

Now I just need to clean up the rest of the code and and fix up the rest of the tests.

[CMCDragonkai]

This should be merged into master as it doesn't depend on testnet deployment, and it can support #357.

As for naming... I think we can do this:

clientPort - port of the GRPC client service
agentPort - port of the GRPC agent service
proxyPort -> connectPort - port of the forward proxy port
proxyPort - the ingress & egress port

[tegefaulkes]

Most tests are succeeding. Some of the tests of the combined Proxy.tests.ts has a very weird error UTP_ECONNRESET seemingly coming from nowhere.

I should be able to finish this off tomorrow.

[CMCDragonkai]

Where those errors there prior to merging the proxies? You should ensure that the merging the proxies doesn't introduce new error behaviour.

On 21 March 2022 6:58:06 pm AEDT, Brian Botha @.***> wrote:

Most tests are succeeding. Some of the tests of the combined Proxy.tests.ts has a very weird error UTP_ECONNRESET seemingly coming from nowhere.

I should be able to finish this off tomorrow.

-- Reply to this email directly or view it on GitHub: https://github.com/MatrixAI/js-polykey/issues/360#issuecomment-1073570325 You are receiving this because you were mentioned.

Message ID: @.***> -- Sent from my Android device with K-9 Mail. Please excuse my brevity.

[tegefaulkes]

It's proving very tricky to narrow down where this error is coming from. The error doesn't occur in the old Reverse Proxy code. I'm thinking it has something to do with the forward proxy interfering in some way but even commenting out seemingly all of the forward proxy aspect hasn't removed the error.

The main problem is that the error provides no usable call stack. I'm assuming it's an event in one of the sockets that isn't being handled. but it's very difficult to tell where.

[CMCDragonkai]

Can you provide a test log of where that error is being output?

And which specific test function is doing it.

[CMCDragonkai]

That error is coming from inside utp-native, it will report it whenever there's some incorrect behaviour involving the UTP connection or UTP socket usually during its lifecycle process.

[tegefaulkes]

I think I've found the problem. Fixing it now.

[tegefaulkes]

All the tests are fixed now. going to apply that name change now.

Just a note. the problem was caused by trying to connect to 0.0.0.0 in the proxy tests. this caused a very vague error with no call stack, Example below.

Error: Unhandled error. (Error: UTP_ECONNRESET
    at createUTPError (/home/faulkes/matrixcode/polykey/js-polykey/node_modules/utp-native/lib/connection.js:238:15)
    at Connection.Object.<anonymous>.Connection._onerror (/home/faulkes/matrixcode/polykey/js-polykey/node_modules/utp-native/lib/connection.js:175:16) {
  code: 'UTP_ECONNRESET',
  errno: 1
})
Error [ERR_UNHANDLED_ERROR]: Unhandled error. (Error: UTP_ECONNRESET
    at createUTPError (/home/faulkes/matrixcode/polykey/js-polykey/node_modules/utp-native/lib/connection.js:238:15)
    at Connection.Object.<anonymous>.Connection._onerror (/home/faulkes/matrixcode/polykey/js-polykey/node_modules/utp-native/lib/connection.js:175:16) {
  code: 'UTP_ECONNRESET',
  errno: 1
})
    at Connection.emit (events.js:364:17)
    at Connection.Object.<anonymous>.Connection._onclose (/home/faulkes/matrixcode/polykey/js-polykey/node_modules/utp-native/lib/connection.js:169:25)

So keep an eye out for that.

[tegefaulkes]

This is pretty much done. I just need to do a quick review on it to check if I've missed anything. I will create a PR and merge this in tomorrow.

[CMCDragonkai]

That's great! In the future you should make a PR first and put the task list into the PR.

[tegefaulkes]

I think I prefer working from the issue. I've found it keeps the larger PRs cleaner and the context of what you're working on more separate.

We can have a chat about it.

[CMCDragonkai]

While you were working on this issue, it was a good idea to have a PR ready, and for your commits/tasks to be put into there, so team members can review and suggest changes as you're addressing the tasks. And issue has no code-context, so no way to refer to tasks to specific parts of the code.

I guess the confusion is exactly what tasks should go into the issue, and what tasks go into the PR.

My workflow right now is that tasks in the issue are light-specifications, but tasks that are in the PR get impacted by the implementation. Tasks in the PR let us know what still needs to be done in the PR, and also what was done in the PR.

Issues should probably stay lighter-weight, and the task list is a prototype/starting point for more detailed tasks in the PR.