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

Projects
None yet
5 participants
@magik6k
Member

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
var CorenetCmd = &cmds.Command{
Helptext: cmds.HelpText{
Tagline: "Application network streams.",

This comment has been minimized.

@whyrusleeping

whyrusleeping May 26, 2017

Member

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

This comment has been minimized.

Member

whyrusleeping commented May 26, 2017

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

This comment has been minimized.

@Kubuxu

Kubuxu May 28, 2017

Member

Use test_must_fail instead of ! here.

@whyrusleeping

This comment has been minimized.

Member

whyrusleeping commented May 30, 2017

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 lgierth and hsanjuan May 30, 2017

@whyrusleeping

This comment has been minimized.

Member

whyrusleeping commented May 30, 2017

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

This comment has been minimized.

Member

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

This comment has been minimized.

Member

Kubuxu commented May 30, 2017

I am also not really attached to the name.

@lgierth

This comment has been minimized.

Member

lgierth commented May 31, 2017

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

tswindell and others added some commits Dec 19, 2016

Experimental corenet application support.
License: MIT
Signed-off-by: Tom Swindell <t.swindell@rubyx.co.uk>
Corenet API: Fixed tests
License: MIT
Signed-off-by: Łukasz Magiera <magik6k@gmail.com>
Corenet API: Use simple util instead of socat for tests
License: MIT
Signed-off-by: Łukasz Magiera <magik6k@gmail.com>
Corenet API: Split list subcmd into ls/streams
License: MIT
Signed-off-by: Łukasz Magiera <magik6k@gmail.com>
Corenet API: Some more tests
License: MIT
Signed-off-by: Łukasz Magiera <magik6k@gmail.com>
Corenet API: Apply suggestions, cleanups
License: MIT
Signed-off-by: Łukasz Magiera <magik6k@gmail.com>
Corenet API: Store state in node
License: MIT
Signed-off-by: Łukasz Magiera <magik6k@gmail.com>
Corenet API: Drop 'exp' namespace
License: MIT
Signed-off-by: Łukasz Magiera <magik6k@gmail.com>
Corenet API: Update deps
License: MIT
Signed-off-by: Łukasz Magiera <magik6k@gmail.com>
Corenet API: Fix codeclimate issues
License: MIT
Signed-off-by: Łukasz Magiera <magik6k@gmail.com>
@magik6k

This comment has been minimized.

Member

magik6k commented May 31, 2017

Should be ready for a review now.

var corenetListenCmd = &cmds.Command{
Helptext: cmds.HelpText{
Tagline: "Create application protocol listener and proxy to network multiaddr.",

This comment has been minimized.

@Kubuxu

Kubuxu Jun 1, 2017

Member

This should have helptext as it is more complex command.

@Kubuxu

Initial CR, I would also like to see more logic of corenet to be moved from commands to the corenet package (commands IMO should be mostly arg parsing and glue code and logic should be in its own package).

It would be also great to have at least part of corenet tested in unittests (using mocked components) which will be possible when more logic is moved from commands to corenet that will require only ipfs/libp2p components it needs.

return
}
addr, peer, err := ParsePeerParam(req.Arguments()[0])

This comment has been minimized.

@Kubuxu

Kubuxu Jun 1, 2017

Member

It should be possible to connect only with PeerID, is that possible with this code?

This comment has been minimized.

@magik6k

magik6k Jun 1, 2017

Member

It is

@@ -47,6 +47,7 @@ ADVANCED COMMANDS
pin Pin objects to local storage
repo Manipulate the IPFS repository
stats Various operational stats
exp Experimental commands

This comment has been minimized.

@Kubuxu

Kubuxu Jun 1, 2017

Member

this doesn't exist anymore and corenet should be added here

}
}
if len(req.Arguments()) != 1 {

This comment has been minimized.

@Kubuxu

Kubuxu Jun 1, 2017

Member

not needed

}
if len(req.Arguments()) != 1 {
}

This comment has been minimized.

@Kubuxu

Kubuxu Jun 1, 2017

Member

Do you remove mux handler anywhere here?

This comment has been minimized.

@magik6k

magik6k Jun 1, 2017

Member

a.Close above invokes corenet/net/net.go/IpfsListener.Close

This comment has been minimized.

@Kubuxu

Kubuxu Jun 1, 2017

Member

Yeah, that is why it would be nice to move all logic to its package. It wasn't clear.

Corenet API: Review fixes
License: MIT
Signed-off-by: Łukasz Magiera <magik6k@gmail.com>
}
// Deregister deregisters protocol handler from this registry
func (c *AppRegistry) Deregister(proto string) {

This comment has been minimized.

@Kubuxu

Kubuxu Jun 2, 2017

Member

This should return error if it can't remove it.

Corenet API: Move more logic away from commands
License: MIT
Signed-off-by: Łukasz Magiera <magik6k@gmail.com>

@magik6k magik6k changed the title from Implemented experimental corenet interface to Implemented experimental ptp(corenet) interface Jun 2, 2017

ptp/apps.go Outdated
}
// AppRegistry is a collection of local application protocol listeners.
type AppRegistry struct {

This comment has been minimized.

@whyrusleeping

whyrusleeping Jun 3, 2017

Member

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 {

This comment has been minimized.

@whyrusleeping

whyrusleeping Jun 3, 2017

Member

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

@lgierth

This comment has been minimized.

Member

lgierth 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

This comment has been minimized.

Member

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

This comment has been minimized.

Member

whyrusleeping commented Jun 3, 2017

magik6k added some commits Jun 2, 2017

PTP API: Rename Corenet to PTP
License: MIT
Signed-off-by: Łukasz Magiera <magik6k@gmail.com>
PTP API: Make code more object oriented, use less node
License: MIT
Signed-off-by: Łukasz Magiera <magik6k@gmail.com>
@whyrusleeping

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
}

This comment has been minimized.

@Kubuxu

Kubuxu Jun 7, 2017

Member

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]

This comment has been minimized.

@Kubuxu

Kubuxu Jun 7, 2017

Member

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
}
}
}

This comment has been minimized.

@Kubuxu

Kubuxu Jun 7, 2017

Member

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

This comment has been minimized.

@Kubuxu

Kubuxu Jun 7, 2017

Member

Resolve this todo.

PTP API: Address review comments
License: MIT
Signed-off-by: Łukasz Magiera <magik6k@gmail.com>
@Kubuxu

Kubuxu approved these changes Jun 9, 2017

@whyrusleeping

This comment has been minimized.

Member

whyrusleeping commented Jun 10, 2017

Here goes! :)

@whyrusleeping whyrusleeping merged commit 18d2a2d into ipfs:master Jun 10, 2017

5 of 7 checks passed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
codeclimate 1 new issue (4 fixed)
Details
ci/circleci Your tests passed on CircleCI!
Details
codecov/patch 74.35% of diff hit (target 63.92%)
Details
codecov/project Absolute coverage decreased by -0.1% but relative coverage increased by +10.42% compared to eef022c
Details
commit-message-check/gitcop All commit messages are valid
Details
continuous-integration/jenkins/pr-merge This commit looks good
Details

@whyrusleeping whyrusleeping added this to the Ipfs 0.4.10 milestone Jun 10, 2017

This was referenced Jun 13, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment