Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implemented experimental ptp(corenet) interface #3943

Merged
merged 15 commits into from
Jun 10, 2017

Conversation

magik6k
Copy link
Member

@magik6k magik6k commented May 26, 2017

This is built on #3518.

TODO:

  • Write some more tests/get good coverage
  • Test in some small real-world use
  • Cleanup the code a bit more

@magik6k magik6k force-pushed the feat/corenet2 branch 3 times, most recently from 283965d to 672f69d Compare May 26, 2017 22:23

var CorenetCmd = &cmds.Command{
Helptext: cmds.HelpText{
Tagline: "Application network streams.",
Copy link
Member

Choose a reason for hiding this comment

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

tagline: "libp2p stream mounting"
shortdescription: "expose a local application to remote peers over libp2p

Note: this command is experimental and subject to change as usecases and APIs are refined"

cc @Kubuxu @lgierth

@whyrusleeping
Copy link
Member

Lets add a config option Experimental.Libp2pStreamMounting and only enable this command if that variable is set to true.

@@ -26,7 +26,16 @@ test_expect_success "test ports are closed" '
(! (netstat -ln | grep "LISTEN" | grep ":10102 "))
'

test_expect_success 'start ipfs listener' '
test_expect_success 'fail without config option being enabled' '
! ipfsi 0 exp corenet ls
Copy link
Member

Choose a reason for hiding this comment

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

Use test_must_fail instead of ! here.

@magik6k magik6k force-pushed the feat/corenet2 branch 2 times, most recently from 795f028 to 7fcb9d0 Compare May 29, 2017 17:32
@whyrusleeping
Copy link
Member

Looking good. The one last thing to consider now is whether or not we should put it under the 'exp' subcommand, or just keep it in the normal namespace. The other experimental commands (filestore and pubsub) exist in the top level, so i'm leaning towards doing that. Also not sure on the naming of 'corenet'.

What do you think @Kubuxu @lgierth @hsanjuan ? (also, @hsanjuan you might be able to rework ipfs cluster to use this feature once its merged :D )

@whyrusleeping whyrusleeping requested review from a user and hsanjuan May 30, 2017 00:02
@whyrusleeping
Copy link
Member

I'm leaning towards having this in the top level namespace (not under an exp command). The whole exp idea came before the config setting anyways.

@magik6k
Copy link
Member Author

magik6k commented May 30, 2017

I still want/need to move all the registry logic to a root package, and make it have instance in node instead of 1 global now. I don't like the naming too, but after trying to come up with something short that would make sense (other than p2pnet, but I'm not sure) I decided to leave corenet, though if you can think of anything that would fit I'll happily change it.

@Kubuxu
Copy link
Member

Kubuxu commented May 30, 2017

I am also not really attached to the name.

@magik6k magik6k force-pushed the feat/corenet2 branch 3 times, most recently from 24d5f04 to 42e9040 Compare May 30, 2017 20:21
@ghost
Copy link

ghost commented May 31, 2017

I'll look at this tomorrow -- awesome work! :)

Tom Swindell and others added 8 commits May 31, 2017 11:33
License: MIT
Signed-off-by: Tom Swindell <[email protected]>
License: MIT
Signed-off-by: Łukasz Magiera <[email protected]>
License: MIT
Signed-off-by: Łukasz Magiera <[email protected]>
License: MIT
Signed-off-by: Łukasz Magiera <[email protected]>
License: MIT
Signed-off-by: Łukasz Magiera <[email protected]>
License: MIT
Signed-off-by: Łukasz Magiera <[email protected]>
License: MIT
Signed-off-by: Łukasz Magiera <[email protected]>
@magik6k magik6k changed the title Implemented experimental corenet interface Implemented experimental ptp(corenet) interface Jun 2, 2017
ptp/apps.go Outdated
}

// AppRegistry is a collection of local application protocol listeners.
type AppRegistry struct {
Copy link
Member

Choose a reason for hiding this comment

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

probably want to add some locking here to make it threadsafe. I can definitely see people running these commands in parallel

ptp/apps.go Outdated
}

// Deregister deregisters protocol handler from this registry
func (c *AppRegistry) Deregister(proto string) error {
Copy link
Member

Choose a reason for hiding this comment

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

will also want to add a listing method that does proper locking

@ghost
Copy link

ghost commented Jun 3, 2017

Awesome work :):) 🌴 I think the use cases here are a bit underexplained, I don't see any showstoppers with the approach taken, but I'm just not clear enough on the use cases :) Is it just a simple 1-to-1 netcat? Or 1-to-n? What is the UX for accepting more than one connection?

Design questions:

  • Why involve listeners and dialers at all? This feels like the layer where you wouldn't deal with IP addresses anymore, only with PeerIDs (or with whatever ipfs swarm connect would accept). And, if you bind an App to a listener here, wouldn't that also involve all other stream handlers too?
  • It's worth considering generalizing this more, to something that just simply manages and exposes stream handlers, connections, and streams. Identify, Bitswap, and the others can be just special cases of that, where go-ipfs itself already has the code to mount them. That'd mean you could adhoc enable or disable the core protocols of IPFS, kill individual streams with individual peers, etc.
  • A propos Identify, will Identify announce what's called Apps here?

I haven't thoroughly CRed the code itself yet, but I think we can keep this whole package free of dependencies on go-ipfs code. There are a few instances where you depend on core.IpfsNode but actually just use its go-libp2p-host.Host and go-libp2p-peer.ID.

@magik6k
Copy link
Member Author

magik6k commented Jun 3, 2017

For now it's more of a tunneling thing (like ssh port forwarding). For more than one connection I'm thinking of a dial option to make it forward more than one connection (so behave like ssh forwarding), perhaps even making it the default behavior and have option for one-time dial.

  • I decided to use dialers as there are no easily muxable API systems yet (websocket/custom proto/etc.). I'm not really sure what you mean by other stream handlers being involved, but the answer is probably no - have you seen ptp/net/net.go?
  • With this code you shouldn't be able to manage built-in stream handlers - the ones used here are prefixed with /app/ prefix(which I will likely change to /ptp/). For now it's only tunnels over libp2p.
  • I don't really know what do you mean by Identify here.

Other than that, some naming conventions are still going to change to make things more clear (app was left from the base PR).

@whyrusleeping
Copy link
Member

whyrusleeping commented Jun 3, 2017 via email

License: MIT
Signed-off-by: Łukasz Magiera <[email protected]>
@magik6k magik6k force-pushed the feat/corenet2 branch 2 times, most recently from 43549df to 712a522 Compare June 3, 2017 21:43
Copy link
Member

@whyrusleeping whyrusleeping left a comment

Choose a reason for hiding this comment

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

This LGTM. like i said earlier, I want to get this in so people who have cool use cases for it can start playing with it. Once we get their feedback we can refine how things work better, and also once we get websockets in the api we can use that as an option.

@lgierth @Kubuxu sound good to you? I'd love to get this into the next release quite soon

if !n.OnlineMode() {
res.SetError(errNotOnline, cmds.ErrClient)
return
}
Copy link
Member

Choose a reason for hiding this comment

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

IMO I would compress code above into once function (returning node or error in case those checks fail) as it is used everywhere.


handlerID, err = strconv.ParseUint(req.Arguments()[0], 10, 64)
if err != nil {
proto = "/ptp/" + req.Arguments()[0]
Copy link
Member

Choose a reason for hiding this comment

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

What if someone creates protocol that is identified by decimal numbers?

If this is a problem right now, please fix it and create sharness test for it.

break
}
}
}
Copy link
Member

Choose a reason for hiding this comment

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

The logic here is quite complex, anyway to make it much simpler to read?

Probably it would be better to completly split off the close all, with handler, and with protocol into separate functions. Maybe even place them in the ptp package.

case "send":
io.Copy(conn, os.Stdin)
default:
//TODO: a bit late
Copy link
Member

Choose a reason for hiding this comment

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

Resolve this todo.

License: MIT
Signed-off-by: Łukasz Magiera <[email protected]>
@whyrusleeping
Copy link
Member

Here goes! :)

@whyrusleeping whyrusleeping merged commit 18d2a2d into ipfs:master Jun 10, 2017
@whyrusleeping whyrusleeping removed the status/in-progress In progress label Jun 10, 2017
@whyrusleeping whyrusleeping added this to the Ipfs 0.4.10 milestone Jun 10, 2017
This was referenced Jun 13, 2017
laurentsenta pushed a commit to laurentsenta/kubo that referenced this pull request Feb 25, 2022
Implemented experimental ptp(corenet) interface
laurentsenta pushed a commit to laurentsenta/kubo that referenced this pull request Feb 25, 2022
Implemented experimental ptp(corenet) interface
laurentsenta pushed a commit to laurentsenta/kubo that referenced this pull request Mar 4, 2022
Implemented experimental ptp(corenet) interface
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants