Refactor `ipfs p2p` #4929

magik6k · 2018-04-10T12:51:36Z

TODO:

Stebalien

Still needs race condition fixes in the registration logic but this is definitely looking better.

Stebalien · 2018-04-17T12:56:21Z

core/commands/p2p.go

@@ -267,7 +267,7 @@ can transparently connect to a p2p service.
 			return
 		}

-		addr, peer, err := ParsePeerParam(req.Arguments()[0])
+		_, peer, err := ParsePeerParam(req.Arguments()[0])


Se should be adding this address to the peerstore (with a temporary TTL) if non-empty.

Stebalien · 2018-04-20T02:56:21Z

p2p/outbound.go

+	}
+
+	switch lnet {
+	case "tcp", "tcp4", "tcp6":


Any reason to check this? I'd just listen and see if it fails.

Stebalien · 2018-04-20T02:57:57Z

p2p/outbound.go

+	ctx, cancel := context.WithTimeout(l.ctx, time.Second*30) //TODO: configurable?
+	defer cancel()
+
+	err := l.p2p.peerHost.Connect(ctx, pstore.PeerInfo{ID: l.peer})


Calling NewStream will implicitly call Connect.

whyrusleeping · 2018-06-01T12:48:57Z

core/commands/p2p.go

-		ShortDescription: "Create and manage listener p2p endpoints",
+		Tagline: "Forward connections to or from libp2p services",
+		ShortDescription: `
+Forward connections to <listen-address> to <target-address>. Protocol specifies


is there a 'from' missing in this sentence?

whyrusleeping · 2018-06-01T14:00:03Z

docs/experimental-features.md

+
+**On the "server" node:**
+
+First, start your application and have it listen on `$APP_PORT`.


maybe clarify that this is a tcp socket being listened on, such as an ssh or http server.

whyrusleeping · 2018-06-01T14:00:28Z

docs/experimental-features.md

+Then, configure the p2p listener by running:
+
+```sh
+> ipfs p2p forward /kickass/1.0 /ipfs /ip4/127.0.0.1/tcp/$APP_PORT


whats the /ipfs in the middle for?

It's the listen address. I could also make it accept the node peerid. I just wanted one command to handle forwarding in both directions.

It would be cleaner if we could specify this as a multiaddress. We'd need to define a multiaddr proto for string protocol names, probably something like /mux/protoName. With that the command wouldn't have to accept magic strings, and the protocol name could get integrated with the address. Creating a libp2p service would look like

ipfs p2p forward /mux/someProto /ip4/127.0.0.1/tcp/$APP_PORT

And forwarding local connections to a service:

ipfs p2p forward /ip4/127.0.0.1/tcp/$APP_PORT /ipfs/QmPeer/mux/someProto

Does that make more sense?

edit: typo: s/ipfs p2p/ipfs p2p forward/

Yeah, that makes sense... Its just a bit confusing. It would be nice to be able to make that simpler somehow. @Stebalien @Kubuxu @schomatis thoughts?

I'm getting the error Error: protocol name must be within '/p2p/' namespace while trying to replicate the example (to better understand this conversation as I don't really know much about libp2p).

Ok, I get it now, changing /kickass/1.0 to /p2p/kickass/1.0 fixes that and the netcat example is working.

oh, should update that too (edit: done)

So, going back to the original question, what seems confusing to me (again, I know nothing about libp2p so feel free to ignore this comment) is to have the p2p port in the front, which seems as if the forwarding is happening only at the p2p layer when in fact the message is routed through different layers (p2p-TCP).

If there were only two arguments (as suggested by @magik6k's solution that for me would be the ideal case) then the from-to relationship would become more evident, but if that isn't possible at the moment I would rather have two different commands that would clearly indicate to the user when the message is received at the p2p layer and routed to the TCP layer (and when the other way around); or at least I would reorganize the command arguments to group the p2p protocol with the p2p address.

whyrusleeping · 2018-06-01T14:05:25Z

p2p/local.go

+	p2p *P2P
+	id  peer.ID
+
+	proto string


maybe make this the protocol.ID type?

whyrusleeping · 2018-06-01T14:07:15Z

p2p/listener.go

+// ListenerRegistry is a collection of local application proto listeners.
+type ListenerRegistry struct {
+	Listeners map[listenerKey]Listener
+	lk        *sync.Mutex


why a pointer to a mutex?

djdv · 2018-06-01T14:13:44Z

core/commands/p2p.go

+Forward connections to <listen-address> to <target-address>. Protocol specifies
+the libp2p protocol to use.
+
+To create libp2p service listener, specify '/ipfs' as <listen-address>


To create a libp2p service listener

djdv · 2018-06-01T14:17:58Z

core/commands/p2p.go

-		"ls":    p2pListenerLsCmd,
-		"open":  p2pListenerListenCmd,
-		"close": p2pListenerCloseCmd,
+		//TODO: Do we really want/need implicit prefix?


Thoughts on this?
Would it cause any restrictions or confusion, with or without them?

It basically restricts one from communicating with libp2p services not in /p2p namespace (so e.g. you can't use this to talk to other peers' bitswap which uses /ipfs/bitswap/1.1.0 as protocol name). The restriction was there before (probably put there by me), but now I'm not sure if we really want/need it.

It probably makes sense to make the user always type /p2p/foo even if we restrict them to that namespace. That way, in the future, if we remove that restriction, we don't have to do anything weird.

djdv · 2018-06-01T14:38:14Z

docs/experimental-features.md

+
+First, configure the p2p dialer to forward all inbound connections on
+`127.0.0.1:SOME_PORT` to the listener behind `/p2p/kickass/1.0` on the server
+node.


First, configure the p2p dialer to forward all inbound connections on 127.0.0.1:SOME_PORT to the listener behind /p2p/kickass/1.0 on the server node.

It may be fine as is, but I wonder if this is more clear.

First, configure the client p2p dialer, so that it forwards all inbound connections on 127.0.0.1:SOME_PORT to the server node listening on /p2p/kickass/1.0.

djdv · 2018-06-01T15:34:55Z

core/commands/p2p.go

 	},
+	Arguments: []cmdkit.Argument{
+		cmdkit.StringArg("protocol", true, false, "Protocol identifier."),
+		cmdkit.StringArg("listen-address", true, false, "Listening endpoint"),


Missing full stop

"Listening endpoint."

djdv · 2018-06-01T15:54:12Z

core/commands/p2p.go

-		cmdkit.StringArg("Protocol", true, false, "Protocol identifier."),
-		cmdkit.StringArg("BindAddress", false, false, "Address to listen for connection/s (default: /ip4/127.0.0.1/tcp/0)."),
+	Options: []cmdkit.Option{
+		cmdkit.BoolOption("headers", "v", "Print table headers (HagndlerID, Protocol, Local, Remote)."),


HagndlerID -> HandlerID

djdv · 2018-06-01T16:07:10Z

core/commands/ping.go

@@ -182,9 +182,11 @@ func pingPeer(ctx context.Context, n *core.IpfsNode, pid peer.ID, numPings int)
 }

 func ParsePeerParam(text string) (ma.Multiaddr, peer.ID, error) {
-	// to be replaced with just multiaddr parsing, once ptp is a multiaddr protocol
-	idx := strings.LastIndex(text, "/")
+	idx := strings.LastIndex(text, "/ipfs/")


There's a few instances of the literal string "/ipfs/" in the package, would it be better to declare it as a named const?
e.g. peerPrefix or something

Ideally we'd parse those as a multiaddr, but for that we'd need to define new multiaddr proto for that.

/ipfs is a multiaddr protocol. That comment may be old. Actually, I believe this entire function is old. Use ParseString from the go-ipfs-addr package.

djdv · 2018-06-01T16:14:33Z

p2p/remote.go

+	p2p *P2P
+
+	// Application proto identifier.
+	proto string


If protocol.ID type is used in local, change must be made here too

djdv · 2018-06-01T16:21:56Z

p2p/local.go

+	return l.p2p.peerHost.NewStream(l.ctx, l.peer, protocol.ID(l.proto))
+}
+
+func (l *localListener) acceptConns() {


Not in this PR, but there's opportunity for logging errors inside of here. Maybe later if a p2p log-subsystem is added.

whyrusleeping · 2018-06-02T07:16:22Z

@magik6k It would be great if you could write up a separate document/tutorial on using ipfs p2p to tunnel ssh connections. I think a specific actionable example would really make the possibilities here click for a lot of people

whyrusleeping · 2018-06-03T09:30:04Z

@magik6k cool, looking pretty good. Last thing I want (aside from getting approvals from @Kubuxu and one other) is a comment I can link to explaining how existing users can update their workflow to work with the changed API. Probably worth posting that comment here: #3994 and then updating my main comment to reflect these changes

magik6k · 2018-06-03T12:45:24Z

Rebased + Updated the comment and added a summary in #3994 (comment)

License: MIT Signed-off-by: Łukasz Magiera <magik6k@gmail.com>

magik6k · 2018-09-11T03:10:45Z

Rebased

Stebalien · 2018-09-11T20:02:04Z

p2p/remote.go

+}
+
+func (l *remoteListener) Close() error {
+	_, err := l.p2p.ListenersP2P.Deregister(string(l.proto))


Now that we're just registering by protocol, this is going to have that close race condition we discussed. That is, if I try to close a listener forwarding to a specific local address twice while someone else is trying to open a listener (to a different local address), I can close their listener.

I believe the correct fix is to just do everything inside the listener manager. That is, no listener handles, etc. Just add a Close(...) method to the listener's object that takes the close "constraints" (protocol, localAddr, remoteAddr).

Does that make sense? This is what I meant by reducing some of the abstractions.

License: MIT Signed-off-by: Łukasz Magiera <magik6k@gmail.com>

magik6k · 2018-09-12T22:40:29Z

I've added Listeners.Close, looks like an improvement. I've also tried to merge Listeners and Listener further, but it resulted in +100 messy lines of duplicated code, so that got hit with reset --hard.

Stebalien · 2018-09-13T23:57:39Z

🎉

I think this has received enough review (4) and we haven't changed the interfaces in most of this review so I'm going to go ahead and merge it. It's not too late to make changes but we should try to make them before the next release (in case anyone gets cold feet and wants to change this).

Stebalien · 2018-09-13T23:58:12Z

Also, @magik6k: ❤️ ❤️ ❤️ ❤️ ❤️ ❤️ ❤️

magik6k requested a review from Kubuxu as a code owner April 10, 2018 12:51

ghost assigned magik6k Apr 10, 2018

ghost added the status/in-progress In progress label Apr 10, 2018

Stebalien reviewed Apr 20, 2018

View reviewed changes

magik6k mentioned this pull request May 23, 2018

[bug] TCP tunnel breaks after every request #5032

Open

magik6k force-pushed the misc/cleanup-p2p branch from 7d97f04 to ba68150 Compare May 26, 2018 14:16

magik6k changed the title ~~[WIP] Cleanup ipfs p2p~~ [WIP] Refactor ipfs p2p May 26, 2018

magik6k force-pushed the misc/cleanup-p2p branch 2 times, most recently from a6db350 to 768d2a0 Compare May 26, 2018 14:35

magik6k changed the title ~~[WIP] Refactor ipfs p2p~~ Refactor ipfs p2p May 26, 2018

whyrusleeping reviewed Jun 1, 2018

View reviewed changes

p2p/local.go Outdated

p2p *P2P

id peer.ID

proto string

Copy link

Member

whyrusleeping Jun 1, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe make this the protocol.ID type?

whyrusleeping reviewed Jun 1, 2018

View reviewed changes

djdv reviewed Jun 1, 2018

View reviewed changes

magik6k mentioned this pull request Jun 3, 2018

ipfs p2p feedback thread #3994

Open

magik6k force-pushed the misc/cleanup-p2p branch from 778930b to 65dd899 Compare June 3, 2018 12:45

magik6k force-pushed the misc/cleanup-p2p branch 2 times, most recently from ca4ca83 to 39f6ab2 Compare June 6, 2018 11:28

magik6k added 17 commits September 11, 2018 04:57

p2p: test to ensure closing right linsters

5e8725d

License: MIT Signed-off-by: Łukasz Magiera <magik6k@gmail.com>

p2p: fix imports after rebase

3c6a168

License: MIT Signed-off-by: Łukasz Magiera <magik6k@gmail.com>

p2p: more locks

8849193

License: MIT Signed-off-by: Łukasz Magiera <magik6k@gmail.com>

p2p: tag connections in connection manager

4badcdc

License: MIT Signed-off-by: Łukasz Magiera <magik6k@gmail.com>

p2p: use host.SetStreamHandlerMatch for now

0f9d284

License: MIT Signed-off-by: Łukasz Magiera <magik6k@gmail.com>

p2p: rebase updates

35eaa16

License: MIT Signed-off-by: Łukasz Magiera <magik6k@gmail.com>

p2p: test for double-registering listeners

8756764

License: MIT Signed-off-by: Łukasz Magiera <magik6k@gmail.com>

p2p: hold lock when handling new streams

d184433

License: MIT Signed-off-by: Łukasz Magiera <magik6k@gmail.com>

p2p: test custom protocol

a5e5b5b

License: MIT Signed-off-by: Łukasz Magiera <magik6k@gmail.com>

p2p: separate listener types

dd48b82

License: MIT Signed-off-by: Łukasz Magiera <magik6k@gmail.com>

p2p: cleanup after listener iface split

bba2d05

License: MIT Signed-off-by: Łukasz Magiera <magik6k@gmail.com>

p2p: fix connmgr use

c509050

License: MIT Signed-off-by: Łukasz Magiera <magik6k@gmail.com>

p2p: deduplicate some listeners logic

228a71a

License: MIT Signed-off-by: Łukasz Magiera <magik6k@gmail.com>

p2p: simplify remote handler matching

05bc3bd

License: MIT Signed-off-by: Łukasz Magiera <magik6k@gmail.com>

p2p: fix docstrings

80b8940

License: MIT Signed-off-by: Łukasz Magiera <magik6k@gmail.com>

p2p: simplify listener startup

f5cb640

License: MIT Signed-off-by: Łukasz Magiera <magik6k@gmail.com>

p2p: rebase import updates

0b575bc

License: MIT Signed-off-by: Łukasz Magiera <magik6k@gmail.com>

magik6k force-pushed the misc/cleanup-p2p branch from 61af105 to 0b575bc Compare September 11, 2018 02:59

Stebalien reviewed Sep 11, 2018

View reviewed changes

p2p: Close on Listeners

b53936a

License: MIT Signed-off-by: Łukasz Magiera <magik6k@gmail.com>

magik6k requested a review from Stebalien September 13, 2018 16:45

Stebalien approved these changes Sep 13, 2018

View reviewed changes

Stebalien merged commit 2de6163 into master Sep 13, 2018

ghost removed the status/in-progress In progress label Sep 13, 2018

Stebalien deleted the misc/cleanup-p2p branch September 13, 2018 23:58

jacobheun mentioned this pull request Jan 22, 2019

feature: p2p stream ipfs/js-ipfs#1828

Closed


		On the "server" node:

		First, start your application and have it listen on `$APP_PORT`.

Refactor ipfs p2p #4929

Refactor ipfs p2p #4929

Conversation

magik6k commented Apr 10, 2018 • edited Loading

Stebalien left a 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

magik6k Jun 2, 2018 • 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

magik6k Jun 3, 2018 • 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

whyrusleeping commented Jun 2, 2018

whyrusleeping commented Jun 3, 2018

magik6k commented Jun 3, 2018

magik6k commented Sep 11, 2018

Choose a reason for hiding this comment

magik6k commented Sep 12, 2018

Stebalien commented Sep 13, 2018

Stebalien commented Sep 13, 2018

Refactor `ipfs p2p` #4929

Refactor `ipfs p2p` #4929

magik6k commented Apr 10, 2018 •

edited

Loading

magik6k Jun 2, 2018 •

edited

Loading

magik6k Jun 3, 2018 •

edited

Loading