NAT proxy for OpenVPN by zolia · Pull Request #850 · mysteriumnetwork/node

zolia · 2019-04-03T14:29:33Z

Implements NATProxy for OpenVPN.

if '--experiment-natpunching' is enabled and port auto-configuration fails node starts sending NAT pings
backwards compatible with clients without NATPinger functionality (or disabled)
NATProxy implementation
NATProxy for TCP protocol is bypassed
Removed restartable openvpn service code since not required now

What lacks:

Think how to test this
More testing with actual clients
Speed tests

donce · 2019-04-17T08:02:45Z

+	case *traversal.NoopPinger:
+		log.Infof("noop pinger detected, returning nil client config.")
+		return nil, nil
+	}


It looks like configuration depends on implementation of traversal.Pinger - maybe it should provide the config to avoid type-conditionals?

Not sure if it is a bad solution, since introducing another config dependency will additionally span throughout the code. Does if worth it?

soffokl · 2019-04-18T08:17:29Z

+	Parse(config json.RawMessage) (ip string, port int, serviceType services.ServiceType, err error)
+}
+
+type portSupplier interface {


We gave this interface in multiple places. Can't we define it once somewhere?
There are many examples of such definitions in a standard library: https://golang.org/pkg/io/#Reader

soffokl · 2019-04-18T08:20:06Z

+
+// Params contains session parameters needed to NAT ping remote peer
+type Params struct {
+	RequestConfig json.RawMessage


Why is it called RequesConfig?
Is it a config of the request?

This config comes with session setup request.

Co-Authored-By: zolia <antanas@mysterium.network>

Waldz · 2019-04-18T11:31:29Z

 			}

-			log.Infof("%s ping target received: IP: %v, port: %v", prefix, IP, port)
+			log.Infof("%sping target received: IP: %v, port: %v", prefix, IP, port)


prefix missing

its not, its format string, prefix at the end.

vkuznecovas

I'm not sure about the moving of the ServicePortSupplier interface to a single spot and then having it referenced in other packages. You're adding hard coupling between the packages where there need not be any. I know it ain't DRY, but I'd rather have less coupling between the packages as you'll end up in a cyclic reference type of situation soon if you do that a lot.

But if everyone else is happy with this - I'm not going to block.

vkuznecovas · 2019-04-18T11:47:16Z

 	"testing"

+	"github.com/mysteriumnetwork/node/core/port"
+


this whitespace is still redundant

tadaskay · 2019-04-18T20:11:48Z

 		proposal := openvpn_discovery.NewServiceProposalWithLocation(currentLocation, transportOptions.Protocol)
+
+		var portSupplier port.ServicePortSupplier = di.PortPool
+		if transportOptions.Port != 0 && locationInfo.PubIP == locationInfo.OutIP {


I'm a bit surprised nobody noticed. This actually changes behavior in most cases (because most clients will have a router), and openvpn.port flag/option is ignored and thus it's always randomized (which causes problems for desktop clients, so for now we decided to use static port 1194 - passing it through options when starting service via tequilapi).

Why was this check needed: locationInfo.PubIP == locationInfo.OutIP? I can't find any useful info in commit messages. Any ideas, @zolia , @vkuznecovas ?

locationInfo.PubIP == locationInfo.OutIP supposed to check if we are behind the NAT.
If IP on the host and public IP is the same, we, probably, don't need to do any NAT traversal since IP is reachable without it.

True, but I think we should respect user flags despite this.

zolia · 2019-04-19T06:18:18Z

You're adding hard coupling between the packages where there need not be any

For future reference. I see upsides and downsides of such solution. In this case, if we have package which is very very feature isolated and self sustained. I think such packageS could expose more generic interfaces. In this case port package IMHO is exactly such a package.

zolia requested review from tadovas and vkuznecovas as code owners April 3, 2019 14:29

Waldz requested changes Apr 5, 2019

View reviewed changes

Comment thread nat/traversal/pinger.go Outdated

Comment thread services/openvpn/service/factory.go Outdated

zolia force-pushed the feature/MYST-845-nat-proxy-openvpn branch 2 times, most recently from 5edea98 to 2ed7fa8 Compare April 15, 2019 15:49

zolia changed the title ~~POC: initial natproxy frame~~ NAT proxy for OpenVPN Apr 16, 2019

zolia force-pushed the feature/MYST-845-nat-proxy-openvpn branch 2 times, most recently from 1acfc01 to 6228ea7 Compare April 17, 2019 08:04

donce reviewed Apr 17, 2019

View reviewed changes

vkuznecovas suggested changes Apr 17, 2019

View reviewed changes

zolia force-pushed the feature/MYST-845-nat-proxy-openvpn branch from 6228ea7 to 7ac6338 Compare April 18, 2019 07:54

zolia requested a review from soffokl as a code owner April 18, 2019 07:54

initial natproxy frame

79b0498

zolia force-pushed the feature/MYST-845-nat-proxy-openvpn branch from 7ac6338 to 79b0498 Compare April 18, 2019 08:10

zolia requested review from Waldz, donce and vkuznecovas April 18, 2019 08:24

pinger stop got back

af29200

zolia force-pushed the feature/MYST-845-nat-proxy-openvpn branch from e8160ac to af29200 Compare April 18, 2019 08:26

soffokl reviewed Apr 18, 2019

View reviewed changes

soffokl and others added 2 commits April 18, 2019 12:24

Update nat/traversal/nat_proxy.go

91b33a3

Co-Authored-By: zolia <antanas@mysterium.network>

localised portSupplier replaced by port.ServicePortSupplier

590643f

zolia requested a review from soffokl April 18, 2019 10:03

Waldz reviewed Apr 18, 2019

View reviewed changes

Waldz approved these changes Apr 18, 2019

View reviewed changes

vkuznecovas approved these changes Apr 18, 2019

View reviewed changes

zolia merged commit fb392b7 into master Apr 18, 2019

zolia deleted the feature/MYST-845-nat-proxy-openvpn branch April 18, 2019 11:55

tadaskay reviewed Apr 18, 2019

View reviewed changes

Conversation

zolia commented Apr 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vkuznecovas left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tadaskay Apr 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zolia commented Apr 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

zolia commented Apr 3, 2019 •

edited

Loading

tadaskay Apr 18, 2019 •

edited

Loading

zolia commented Apr 19, 2019 •

edited

Loading