refactor: combined `ForwardProxy` and `ReverseProxy` into `Proxy` #361

tegefaulkes · 2022-03-23T00:18:33Z

Description

ForwardProxy and ReverseProxy has been conbined into a single Proxy. the core part of this change is that both the proxies now share the same UTP socket. This allows us to get information about the reverse proxy's port when handling an incoming connection. This is critical for NAT punch-through procedure.

I can just merge the branch manually but I want to do a proper review quickly and leave a paper trail. SO it's best to have this as a proper PR.

Issues Fixed

Fixes Refactor proxies to use the same UTP socket for external connections #360
Relates to Testnet Deployment #326

Tasks

review

Final checklist

CMCDragonkai · 2022-03-23T01:45:41Z

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└────────────┘

CMCDragonkai · 2022-03-23T01:48:11Z

Now you can do things like:

getConnectionInfoByProxy - get connection info by the remote proxy host and port
getConnectionInfoByClient - get connection info by the GRPC client host and port
getConnectionInfoByReverse - get connection info by the reverse host and port used by GRPC server handlers

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

src/network/Proxy.ts

tegefaulkes

Just some general stuff to fix.

src/network/ConnectionReverse.ts

src/network/Proxy.ts

tests/bin/agent/start.test.ts

tests/nodes/NodeConnectionManager.termination.test.ts

tests/nodes/NodeManager.test.ts

src/network/types.ts

CMCDragonkai · 2022-03-23T02:17:13Z

I can see you're new commit standard, are you following the https://www.conventionalcommits.org/en/v1.0.0/#summary standard now? I haven't had the time to bring in the commit linting into the CI/CD at this point.

src/network/ConnectionReverse.ts

src/network/errors.ts

src/nodes/NodeConnectionManager.ts

CMCDragonkai · 2022-03-23T02:27:21Z

I can see you're new commit standard, are you following the https://www.conventionalcommits.org/en/v1.0.0/#summary standard now? I haven't had the time to bring in the commit linting into the CI/CD at this point.

Looking at the conventional commits PRs https://github.com/conventional-commits/conventionalcommits.org/pulls, it seems that the title of the PR can be in the conventional commit structure as well, but it doesn't need to match the commit name if there are multiple commits, it can be its own message. This is because the PR title might become the merge commit message or part of it. So if you have 2 commits, then the PR title might synthesise a title from both.

src/network/Proxy.ts

tegefaulkes · 2022-03-23T04:38:59Z

CI/CD is passing now. I'll start squashing.

CMCDragonkai · 2022-03-23T04:59:08Z

@emmacasolin note that now when running pk agent start, you'd use --proxy-host and --proxy-port in place of --ingress-host and --ingress-port.

LGTM, so we can merge once CI/CD.

CMCDragonkai · 2022-03-23T05:00:19Z

Commit message typos, just need to fix that up.

`ForwardProxy` and `ReverseProxy` has been combined into a single `Proxy`. The core part of this change is that both the proxies now share the same `UTP` socket. This allows us to get information about the reverse proxy's port when handling an incoming connection. This is critical for `NAT` punch-through procedure. Fixes #360

emmacasolin · 2022-03-28T23:02:10Z

I've just noticed something that was missed here. The constructor for the new Proxy takes

connConnectTime?: number;
connKeepAliveTimeoutTime?: number;
connEndTime?: number;
connPunchIntervalTime?: number;
connKeepAliveIntervalTime?: number;

However the proxyConfig object in both config.ts and PolykeyAgent.ts takes

connConnectTime?: number;
connTimeoutTime?: number;
connPingIntervalTime?: number;

So setting any values for connTimeoutTime or connPingIntervalTime when creating a new polykey agent doesn't actually do anything since these are no longer parameters for the Proxy constructor.

Also related to this issue (but not brought in by this PR) is that the --connection-timeout option for pk agent start doesn't work at the moment. This is because we're attempting to access the value of that option inside CommandStart.ts using options.connTimeoutTime, however commander actually stores the value in options.connectionTimeout.

CMCDragonkai · 2022-03-29T00:10:31Z

Keep in mind with combine forward and reverse any timeout and interval settings is duplicated for both forward and reverse. This is fine, forward and reverse should have similar times for both timeout and interval. At least until we see a problem with it.

CMCDragonkai · 2022-03-29T02:37:40Z

@tegefaulkes can you push hotpatch to master regarding @emmacasolin report above. That way #357 can just directly rebase on top of master now instead of #326.

tegefaulkes · 2022-03-29T02:40:46Z

I'll do that. I'll fix both things that were mentioned here?

CMCDragonkai · 2022-03-29T02:49:28Z

Yep.

tegefaulkes · 2022-03-29T04:20:40Z

Pushed up the fix to master.

tegefaulkes added the development Standard development label Mar 23, 2022

tegefaulkes self-assigned this Mar 23, 2022