-
Notifications
You must be signed in to change notification settings - Fork 4
Conversation
c3b9607
to
7171f76
Compare
5cc24aa
to
2455d11
Compare
cb5b271
to
d115119
Compare
d115119
to
a159564
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So the js-libp2p-daemon already supports a good chunk of this. Would it be worth just adding docker support there instead of maintaining the 2 of these? We might have to make some tweaks to support not needing a local control connection, but that might be a better consolidation of effort.
src/index.js
Outdated
transport: [Websockets, TCP], | ||
streamMuxer: [Muxer], | ||
connEncryption: [Crypto], | ||
contentRouting |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-
I think we should just drop advertise support for now in here until we fix the DHT. The relay won't be auto discoverable but being able to quickly run a standalone, static relay is still valuable. The external dependency doesn't really seem worth it to me right now.
-
This should also use pubsub peer discovery https://github.com/libp2p/js-libp2p-pubsub-peer-discovery.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I removed the advertise stuff and added the pubsub discovery with options in the CLI for it
src/index.js
Outdated
enabled: true, // Allows you to dial and accept relayed connections. Does not make you a relay. | ||
hop: { | ||
enabled: true, // Allows you to be a relay for other peers | ||
active: true // You will attempt to dial destination peers if you are not connected to them |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Disable active, support needs to be added, https://github.com/libp2p/js-libp2p/blob/master/src/circuit/circuit/hop.js#L53, this should also be configurable by users.
This is a good point. I was looking here into an out of the box solution for spinning up a relay, in order to enable someone to easily setup it without any prior knowledge needed on js-libp2p configuration. The big limitation here is that we cannot have multiple docker files in a single github repository totally integrated in Docker Hub for things like automated builds as the name of the image is the name of the repo. Otherwise, we could have a set of images in a single repo. If we go with the daemon, we would need to have a more sane default setup for a typical libp2p node that you could run on docker. As a consequence, the configuration needed to run the container would be way more complex. Moreover, this module can also be used as a lib with the libp2p API instead of the daemon client. With the above in mind, I think it makes sense to have it as a separate repo and I would not expect much maintenance overhead here with dependabot |
2a8f0e8
to
34df221
Compare
The daemon should have sane defaults anyway. The main thing it's missing is pubsub discovery, it already enables hop by default. FWIW, the few relays libp2p currently runs on the public network use the go daemon.
The main advantage of this is being able to require a module that's good to go, but tweaking configuration isn't trivial. Thinking about preconfigured setups a bit more, how would you want to handle a variety of these? In their own modules? A mono repo? I don't want to block this, as we can always archive it if it get's stale, but thinking about how and where preconfigured builds should exist, especially as we revamp configuration in libp2p is something we should spend some time thinking about. |
Dockerfile
Outdated
@@ -0,0 +1,28 @@ | |||
FROM node:lts-buster |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the size of this image?
I'd take a look at libp2p/js-libp2p-webrtc-star#223 to see the improvements made there. Buster likely has a lot more than is needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very smaller this one! thanks
README.md
Outdated
DISCOVERY_TOPICS='_peer-discovery._app1._pubsub,_peer_discovery._app2._pubsub' | ||
``` | ||
|
||
Please note that you should expose expose the used ports with the docker run command. The default ports used are `8003` for the metrics and `150003` for the websockets listener |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please note that you should expose expose the used ports with the docker run command. The default ports used are `8003` for the metrics and `150003` for the websockets listener | |
Please note that you should expose the used ports with the docker run command. The default ports used are `8003` for the metrics, `8000` for the TCP transport, and `15002` for the Websockets transport |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, I don't see any defaults aside from metrics. The docker commands include them, but there aren't defaults in the code unless I am missing something.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, perhaps the example below created confusion. I added now example
before it.
So, the default is in the getListenAddresses
and is '/ip4/127.0.0.1/tcp/15003/ws'. It is also mentioned above in the readme on the multiaddrs subsection. Should I also add a default tcp multiaddr?
|
||
```sh | ||
libp2p-relay-server --discoveryTopics '_peer-discovery._app1._pubsub' '_peer_discovery._app2._pubsub' | ||
``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd just leave configuring this out, keep it simple. It binds to the global space out of the box, so just avoid people doing the wrong thing. IMO if people need to configure this in depth they can use the config as a template. The point of this repo is basically to get people up and running quickly, so we should just reduce the noise.
What's the bare minimum I need to do to get this running? The readme should focus on that. I think a consistent PeerId and Announce addresses are the only thing I should actually need to set. I'd prioritize and separate the need to have, from the optional stuff.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I divided them in two now. The required/recommended is in the usage section: peerId
, multiaddrs
and ssl
Then a new configuration section created for fine tuning of the image
README.md
Outdated
|
||
### SSL | ||
|
||
You should setup an SSL certificate with nginx and proxy to the API. You can use a service that already offers an SSL certificate with the server and configure nginx, or you can create valid certificates with for example [Letsencrypt](https://certbot.eff.org/lets-encrypt/osx-nginx). Letsencrypt won’t give you a cert for an IP address (yet) so you need to connect via SSL to a domain name. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not needed right now, but it would be cool to see an example dockerfile extending this one, templated with nginx and letsencrypt.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this would be good! I added it in the libp2p production guides README libp2p/js-libp2p#804 (comment) as it should probably be not specific to the relay. We can also link it from here afterwards.
I also agree that we should have a ready to go docker container for the daemon. I did not remember that the daemon had hop enabled by default. What is the reasoning behind it? matching go? I think that setting up a js daemon out of the box should match exactly js libp2p defaults, which is not the case here.
This is really something we should think about. It would be great if we could provide at the same time preconfigured builds and libp2p configurations. Libp2p configurations could be a mono repo. Once we do the configuration improvements, we need to have ready to go configurations. We need to check if we can have multiple Dockerfiles in a lerna mono repo with automated builds, but as far as I know this is not possible at the moment If we could do that, a mono repo with configurations that can be imported and docker images that could be pulled for each configuration. This would enable to use the daemon for the Docker builds and it would be easy to import and use as a module too. If we cannot use a mono repo, this will be difficult to achieve in terms of the builds. Specially thanks to the modularity nature of libp2p, there will be no way to have a docker image where we can simply add/remove a given module. In this case, we should probably have a mono repo with all the configurations and repos with the most common ready to go builds. Specially for things like relay, rendezvous servers, ... Considering for example browser based configurations, they would not need a ready to go build, so it might make sense to just have the configurations in a lerna repo. We should discuss this better later on, I will mention this discussion in the configuration issue |
We may want to look at just making js-libp2p itself a monorepo, akin to js-ipfs, as this would make shipping preconfigured builds much easier. This would also enable us to pull in things like the daemon and daemon client to make that workflow suck less. Can you open up an issue in js-libp2p to discuss this? It would be good to chat through the benefits, drawbacks and lessons learned from js-ipfs in an open context there. I'll finish going through this PR and we can move the monorepo convo there. |
src/index.js
Outdated
const Muxer = require('libp2p-mplex') | ||
const { NOISE: Crypto } = require('libp2p-noise') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd recommend just giving them their names, it makes the config easier to read. We use this format in testing to be generic, we should avoid it outside of testing.
src/utils.js
Outdated
} | ||
|
||
function getListenAddresses (argv) { | ||
let listenAddresses = ['/ip4/127.0.0.1/tcp/15003/ws'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're only doing WS by default? We should be doing TCP as well. Having only 1 really reduces the usefulness of the relay as we won't be able to relay TCP -> WS.
test/index.spec.js
Outdated
await relay.start() | ||
|
||
expect(relay.multiaddrs).to.have.lengthOf(listenAddresses.length) | ||
relay.multiaddrs.forEach((m) => listenAddresses.includes(m.toString())) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't checking anything, it's just iterating and returning a boolean to nowhere.
README.md
Outdated
|
||
```sh | ||
docker build NAME -t libp2p-relay | ||
docker run -p 8003:8003 -p 15002:15002 -p 8000:8000 -e LISTEN_MULTIADDRS='/ip4/127.0.0.1/tcp/15002/ws,/ip4/127.0.0.1/tcp/8000' -d libp2p-relay |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be helpful to show how to include the peer id file here. Setting PEER_ID='./id.json'
will fail because no file has been given to docker. This example is just going to create new peer ids.
Also, if we added announce addresses to the example -e ANNOUNCE_MULTIADDRS='/dns4/localhost/tcp/8000,/dns4/localhost/tcp/15002/ws'
, people should be able to play around with this locally.
Yes, that would be worth discussing! I will open the issue. I would add the interfaces to that list |
Co-authored-by: Jacob Heun <jacobheun@gmail.com>
ae63409
to
6ab04d4
Compare
README.md
Outdated
ANNOUNCE_MULTIADDRS='/dns4/test.io/tcp/443/wss,/dns6/test.io/tcp/443/wss' | ||
``` | ||
|
||
Docker does not provide a clean way of adding a file using the RUN command. As a file should be added before running the server, the easiest way is to create a `Dockerfile` that extends this Docker image and copy a file to a path that you can reference. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't correct, you can mount the file. Here's an example you can just copy paste to get this working, assuming you have a peer id stored at id.json
in your current directory (PWD
). I've run this locally, but haven't tried connecting another node to it though. This could replace the example below.
peer-id --type=ed25519 > id.json
docker run -p 8003:8003 -p 15002:15002 -p 8000:8000 \
-e LISTEN_MULTIADDRS='/ip4/127.0.0.1/tcp/8000,/ip4/127.0.0.1/tcp/15002/ws' \
-e ANNOUNCE_MULTIADDRS='/dns4/localhost/tcp/8000,/dns4/localhost/tcp/15002/ws' \
-e PEER_ID='/etc/opt/relay/id.json' \
-v $PWD/id.json:/etc/opt/relay/id.json \
-d libp2p-relay
Edit: added the peer-id command for good measure
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice thanks! I tried it now and it works, this is a better solution.
This PR adds the initial implementation of an out of the box Libp2p hop relay to be deployed. It is highly customisable and comes with a Docker image.
Context: libp2p/js-libp2p#804