lib/connections: Try TCP punchthrough #5753

AudriusButkevicius · 2019-05-29T20:11:20Z

This is hopefully a PR with only the relevant changes, if I haven't failed to git.

lib/dialer/debug.go

AudriusButkevicius · 2019-07-14T14:54:10Z

Ready for review maybe? Given 1.2.0 is out, and quic has not broken the world, let's try to do it again.

imsodin

Looks good generally, mostly nitpicks below.

Is my understanding of the mechanism correct: We now dial and listen on the same port. We announce an unspecified address to the discovery server, which then uses the port of the announcement. We expect this to be the same port that the NAT assigned to where we are listening/dialing on. The remote then uses that port for dialing, and thus we manage to dial and listen on the same port.

lib/dialer/control_unix.go

lib/dialer/debug.go

lib/util/utils.go

lib/connections/tcp_listen.go

lib/dialer/public.go

lib/dialer/internal.go

lib/connections/tcp_dial.go

lib/dialer/public.go

lib/osutil/ping.go

lib/relay/client/methods.go

lib/dialer/public.go

AudriusButkevicius · 2019-07-15T23:01:05Z

I've addressed the review comments and unmarked some comments as done as it's not for me to judge when it's done.

imsodin

I am totally fine with you dismissing my utterly nit-picky and/or opinionated comments - it's still nice when you let me mark them done just for convenience when reading your answers ;)

I like the sneak attack, but I think there's a problem with how it's done currently. The other two comments are even worse nitpickery than before, ignore at will.

Otherwise this is lgtm from my side. I didn't look up the low-level stuff (syscall), I am sure you got that right.

lib/dialer/control_unix.go

lib/dialer/public.go

lib/relay/client/dynamic.go

AudriusButkevicius · 2019-07-16T07:40:03Z

Editing on the mobile on a train, yay.
Anyways, addressed comments.

Time for @calmh?

calmh

Looks reasonable, I'm going to see if I can do some testing...

lib/relay/client/dynamic.go

lib/dialer/public.go

lib/dialer/debug.go

golangcibot · 2019-07-27T10:34:41Z

lib/dialer/public.go

+	if localAddrInterface != nil {
+		if addr, ok := localAddrInterface.(*net.TCPAddr); !ok {
+			return nil, errUnexpectedInterfaceType
+		} else {


if block ends with a return statement, so drop this else and outdent its block (move short variable declaration to its own line if necessary) (from golint)

You are fucking stupid.

Hey, it's right although you may not agree with the style choice

I struggle to understand where the suggested style choice would be better.

imsodin · 2019-07-27T10:56:24Z

The old go version check (go1.10.7) fails due to ListenConfig and Dialer.Control that have been introduced in go1.11: https://golang.org/doc/go1.11#net. go1.13 will be out soon and by using modules we already have a kind of "soft requirement" for go1.11, so I think we should be fine bumping the old go version check to go1.11.

AudriusButkevicius · 2020-04-29T20:33:29Z

So the announce part seems to work for me.
Added req.RemoteAddr to the typical log:

380704bb7b4d7c03 POST / 86.29.10.111:22000

And log the announcement addresses before and after the "fixup"

Announce: 2ERMYXS-ZKTQICW-AWCYDBJ-ZDSV4JA-5PYZHDV-6Q4COSC-3POAZJO-UDAYFAS got: [tcp://0.0.0.0:0 tcp://0.0.0.0:22000 quic://0.0.0.0:22000] made: [tcp://86.29.10.111:22000 tcp://86.29.10.111:22000 quic://86.29.10.111:22000]

I suspect the problem you've seen is because nginx/caddy fronting the server does not forward the port in X-Forwarded-Port.

AudriusButkevicius · 2020-05-20T21:03:35Z

How do we take this forward?

calmh · 2020-05-25T07:34:04Z

Some kind of reasonable testing I guess. Have you run this and seen it work? If so I'll take it for another spin and we can debug any issues. Last time I had the feeling it was all theoretical and hence I wasn't sure whether to debug my network or just assume that things didn't work as intended...

AudriusButkevicius · 2020-05-25T09:15:32Z

Well the only thing I tested is that the source address connecting to discovery is port 22000, and that discovery correctly substitutes port 0 with that port (on a local discovery server). I have not tested actual punch through capabilities as I don't have two devices behind different domestic routers.

AudriusButkevicius · 2020-06-07T11:07:10Z

I think the best way forward with this would be to cherry-pick and deploy the discovery changes and adding support for X-Forwarded-Port.

This should have no effect.

Then run with this PR for a week to verify it doesn't break anything existing in your setup, and then just merge it on good will and see if anything comes from the RC?

AudriusButkevicius · 2020-06-12T19:13:30Z

@calmh @imsodin bhuump. What are we doing with this?

imsodin

Your plan from a week ago (sorry for the delay) sound good. If it works at all should be well discernable from the transport usage reporting and if it doesn't, reverting is easy (as longs as we don't redesign dialing in the meantime).

imsodin · 2020-06-13T16:32:03Z

cmd/stdiscosrv/apisrv.go

+		if remote != nil {
+			if host == "" || ip.IsUnspecified() {


Could be remote != nil && (host == "" || ip.IsUnspecified()) removing the need for an additional indentation.

There is a block after the nested if which would have to repeat remote != nil then.

AudriusButkevicius · 2020-06-14T13:50:12Z

Soo, approve and lets go? 👯

imsodin · 2020-06-14T16:13:44Z

Only @calmh can deploy, so the ball is in his court now, isn't it?
I might also just run a discovery server at home for testing, I just need to overcome the laziness to set the ports and clients up for that...

calmh · 2020-06-16T06:57:33Z

I actually did deploy the discovery servers from this a while back (still running), but I can do so again. I'm generally not super excited about merging code that the person who wrote it hasn't been able to test at all for whether it actually does what it's supposed to, but if there's confidence that it doesn't sabotage existing connection setup and a plan for actual post-merge testing and reporting back, sure.

calmh · 2020-06-16T07:04:56Z

So looking at this it seems we want X-Forwarded-Port to be the source port of the client, while it's generally meant to be the (original) server port? I'll set it up with the client port

* main: (37 commits) various: Master is now main lib/model: Fix minor flakiness in TestModTimeWindow cmd/ursrv: Attempt to fix js failure to load (syncthing#6747) lib/osutil: Use filepath.Join in tests (syncthing#6740) cmd/strelaysrv: Fix JSON unmarshalling when joining a relay pool (fixes syncthing#6733) (syncthing#6729) gui, man, authors: Update docs, translations, and contributors build(deps): bump github.com/lucas-clemente/quic-go (syncthing#6722) lib/db: Remove unused keyer methods (syncthing#6723) gui: Fix auto dismissing auth notification (ref syncthing#6536) lib/scanner: Save one stat call per file (syncthing#6715) gui: Ignore permissions isn't just about FAT (syncthing#6713) lib/connections, lib/nat: Correctly dis-/enable nat (fixes syncthing#6552) (syncthing#6719) gui: Fix string and update translations (ref syncthing#6536) (syncthing#6716) lib/protocol: Avoid panic in DeviceIDFromBytes (syncthing#6714) build: Fix rebuild of strelaypoolsrv assets cmd/strelaysrv: Add note about relay operators cmd/stcrashreceiver: Pick up extra build tags and send to Sentry (syncthing#6710) lib/build: Add some env vars as synthetic build tags (syncthing#6709) build(deps): bump github.com/lucas-clemente/quic-go (syncthing#6695) gui, man, authors: Update docs, translations, and contributors ...

calmh · 2020-06-16T07:16:20Z

Well some cursory testing at home seems to indicate it doesn't in fact fuck anything up beyond repair, so that's good 👍

AudriusButkevicius · 2020-06-16T07:18:52Z

You are right, I should probably change it to X-Client-Port or something.

I'll also try to come with with something that atleast tries to show it working.

calmh · 2020-06-16T07:21:49Z

I would try it with one end tethered behind mobile, assuming the other is behind a "regular" router. But this still requires full cone (I think? or maybe just deterministic port mappings) so might not work in that setup even if everything is great on our side.

calmh · 2020-06-16T07:22:48Z

Anyway it's deployed server side, though we can redeploy with a different header name later

* main: (30 commits) lib/connections: Try TCP punchthrough (fixes syncthing#4259) (syncthing#5753) build(deps): bump github.com/lucas-clemente/quic-go (syncthing#6742) various: Master is now main lib/model: Fix minor flakiness in TestModTimeWindow cmd/ursrv: Attempt to fix js failure to load (syncthing#6747) lib/osutil: Use filepath.Join in tests (syncthing#6740) cmd/strelaysrv: Fix JSON unmarshalling when joining a relay pool (fixes syncthing#6733) (syncthing#6729) gui, man, authors: Update docs, translations, and contributors build(deps): bump github.com/lucas-clemente/quic-go (syncthing#6722) lib/db: Remove unused keyer methods (syncthing#6723) gui: Fix auto dismissing auth notification (ref syncthing#6536) lib/scanner: Save one stat call per file (syncthing#6715) gui: Ignore permissions isn't just about FAT (syncthing#6713) lib/connections, lib/nat: Correctly dis-/enable nat (fixes syncthing#6552) (syncthing#6719) gui: Fix string and update translations (ref syncthing#6536) (syncthing#6716) lib/protocol: Avoid panic in DeviceIDFromBytes (syncthing#6714) build: Fix rebuild of strelaypoolsrv assets cmd/strelaysrv: Add note about relay operators cmd/stcrashreceiver: Pick up extra build tags and send to Sentry (syncthing#6710) lib/build: Add some env vars as synthetic build tags (syncthing#6709) ...

AudriusButkevicius force-pushed the tcp-reuse branch from 7a3d4da to e7f9794 Compare May 29, 2019 23:04

lib/connections: Reuse TCP port, advertise zero port

461a70b

AudriusButkevicius force-pushed the tcp-reuse branch from e7f9794 to 461a70b Compare May 29, 2019 23:07

syncthing deleted a comment from golangcibot May 29, 2019

golangcibot reviewed May 29, 2019

View reviewed changes

lib/dialer/debug.go Outdated Show resolved Hide resolved

syncthing deleted a comment from golangcibot May 29, 2019

Merge upstream master

7b36ff0

imsodin reviewed Jul 15, 2019

View reviewed changes

AudriusButkevicius added 2 commits July 15, 2019 19:19

Review comments before faffing with dialers

e8a53a2

Remove plenty of dead code, sneak in relay latency optimisation

8c1995f

golangcibot reviewed Jul 15, 2019

View reviewed changes

lib/dialer/public.go Outdated Show resolved Hide resolved

lib/osutil/ping.go Outdated Show resolved Hide resolved

lib/relay/client/methods.go Outdated Show resolved Hide resolved

lib/dialer/public.go Outdated Show resolved Hide resolved

AudriusButkevicius added 2 commits July 15, 2019 23:57

Lint

f5573d5

Always cancel contexts

19158b0

imsodin reviewed Jul 16, 2019

View reviewed changes

lib/dialer/control_unix.go Outdated Show resolved Hide resolved

lib/dialer/public.go Show resolved Hide resolved

lib/relay/client/dynamic.go Outdated Show resolved Hide resolved

AudriusButkevicius added 3 commits July 16, 2019 08:36

Update control_unix.go

d5eac80

Update control_windows.go

167e049

Update control_unsupported.go

6772fd7

imsodin mentioned this pull request Jul 16, 2019

Takes a long time to shut down on Ctrl-C (1.2.1-rc.1) #5860

Closed

calmh reviewed Jul 26, 2019

View reviewed changes

lib/relay/client/dynamic.go Outdated Show resolved Hide resolved

lib/dialer/public.go Outdated Show resolved Hide resolved

lib/dialer/debug.go Outdated Show resolved Hide resolved

AudriusButkevicius added 3 commits July 27, 2019 11:28

Merge upstream/master

f3bea43

Assert types

31a19d3

Merge remote-tracking branch 'origin/tcp-reuse' into tcp-reuse

982455e

golangcibot reviewed Jul 27, 2019

View reviewed changes

AudriusButkevicius added 2 commits April 29, 2020 20:34

Merge master

d92ac57

Merge origin

6026474

AudriusButkevicius and others added 2 commits May 25, 2020 10:35

Merge branch 'master' into tcp-reuse

79179ba

Update tcp_listen.go

fd17241

Update tcp_listen.go

285b631

imsodin reviewed Jun 13, 2020

View reviewed changes

calmh changed the base branch from master to main June 15, 2020 19:31

calmh approved these changes Jun 16, 2020

View reviewed changes

calmh merged commit f619a7f into syncthing:main Jun 16, 2020

calmh added this to the v1.7.1 milestone Jun 16, 2020

GermanCoding mentioned this pull request Jul 15, 2020

users/stdiscosrv: New HTTP header "X-Client-Port" (ref syncthing#5753) syncthing/docs#534

Merged

imsodin mentioned this pull request Jan 16, 2021

Port 0 listen address gets announced to discovery servers #4278

Closed

st-review added the frozen-due-to-age Issues closed and untouched for a long time, together with being locked for discussion label Jun 17, 2021

syncthing locked and limited conversation to collaborators Jun 17, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lib/connections: Try TCP punchthrough #5753

lib/connections: Try TCP punchthrough #5753

AudriusButkevicius commented May 29, 2019

AudriusButkevicius commented Jul 14, 2019

imsodin left a comment

AudriusButkevicius commented Jul 15, 2019

imsodin left a comment

AudriusButkevicius commented Jul 16, 2019

calmh left a comment

golangcibot Jul 27, 2019

AudriusButkevicius Jul 27, 2019

calmh Jul 27, 2019

AudriusButkevicius Jul 27, 2019

imsodin commented Jul 27, 2019

AudriusButkevicius commented Apr 29, 2020 •

edited

Loading

AudriusButkevicius commented May 20, 2020

calmh commented May 25, 2020

AudriusButkevicius commented May 25, 2020

AudriusButkevicius commented Jun 7, 2020

AudriusButkevicius commented Jun 12, 2020

imsodin left a comment

imsodin Jun 13, 2020

AudriusButkevicius Jun 13, 2020

AudriusButkevicius commented Jun 14, 2020 •

edited

Loading

imsodin commented Jun 14, 2020

calmh commented Jun 16, 2020

calmh commented Jun 16, 2020

calmh commented Jun 16, 2020

AudriusButkevicius commented Jun 16, 2020

calmh commented Jun 16, 2020

calmh commented Jun 16, 2020

lib/connections: Try TCP punchthrough #5753

lib/connections: Try TCP punchthrough #5753

Conversation

AudriusButkevicius commented May 29, 2019

AudriusButkevicius commented Jul 14, 2019

imsodin left a comment

Choose a reason for hiding this comment

AudriusButkevicius commented Jul 15, 2019

imsodin left a comment

Choose a reason for hiding this comment

AudriusButkevicius commented Jul 16, 2019

calmh left a comment

Choose a reason for hiding this comment

golangcibot Jul 27, 2019

Choose a reason for hiding this comment

AudriusButkevicius Jul 27, 2019

Choose a reason for hiding this comment

calmh Jul 27, 2019

Choose a reason for hiding this comment

AudriusButkevicius Jul 27, 2019

Choose a reason for hiding this comment

imsodin commented Jul 27, 2019

AudriusButkevicius commented Apr 29, 2020 • edited Loading

AudriusButkevicius commented May 20, 2020

calmh commented May 25, 2020

AudriusButkevicius commented May 25, 2020

AudriusButkevicius commented Jun 7, 2020

AudriusButkevicius commented Jun 12, 2020

imsodin left a comment

Choose a reason for hiding this comment

imsodin Jun 13, 2020

Choose a reason for hiding this comment

AudriusButkevicius Jun 13, 2020

Choose a reason for hiding this comment

AudriusButkevicius commented Jun 14, 2020 • edited Loading

imsodin commented Jun 14, 2020

calmh commented Jun 16, 2020

calmh commented Jun 16, 2020

calmh commented Jun 16, 2020

AudriusButkevicius commented Jun 16, 2020

calmh commented Jun 16, 2020

calmh commented Jun 16, 2020

AudriusButkevicius commented Apr 29, 2020 •

edited

Loading

AudriusButkevicius commented Jun 14, 2020 •

edited

Loading