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

Dockerfile and instructions #227

Merged
merged 2 commits into from
Sep 14, 2023
Merged

Dockerfile and instructions #227

merged 2 commits into from
Sep 14, 2023

Conversation

orbitalturtle
Copy link
Contributor

@orbitalturtle orbitalturtle commented Jul 7, 2023

Thanks to @anmode and @tsjk for starting this Dockerfile work in PR #197 . I just made a couple of changes to get it working on my end, including adding the /.teos directory to the teosd group (otherwise I was getting a permissions error when running the container).

  • It seems that by only copying over the specific binaries we need (teos-cli and teosd) rather than the full /target folder reduced the build to ~100 MB.
  • Changing from debian to alpine further reduced the build to ~40 MB

For the tutorials, thanks to @sr-gi, who I believe wrote the original Teos python Docker documentation much of which I borrowed in this PR. (Though I removed the mention of creating a volume since we automatically do that in the current Dockerfile. Thoughts?) I've added a section for how to set up a Tor hidden service pointing to the Docker instance.

Next steps:

  • One thing that was discussed in PR Create the rust-teos dockerize #197 is to explore how to allow someone to migrate an existing Teos instance to a container. Some changes will be needed to make the existing Tor key compatible and instructions for how to do this need to be added.

(I wrote the Tor Docker instructions after attempting to follow the discussion in #197 . If I got anything wrong, feel free to let me know.)

@mariocynicys mariocynicys self-assigned this Jul 18, 2023
Copy link
Collaborator

@mariocynicys mariocynicys left a comment

Choose a reason for hiding this comment

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

This is missing some docs about teos-cli.
The usage should be like so:

docker exec -it teos teos-cli --help

That's after running teosd in a container named teos (hence the name after -it part)

docker/README.md Outdated Show resolved Hide resolved
docker/README.md Show resolved Hide resolved
docker/Dockerfile Outdated Show resolved Hide resolved
docker/README.md Show resolved Hide resolved
docker/README.md Outdated Show resolved Hide resolved
@mariocynicys
Copy link
Collaborator

mariocynicys commented Jul 19, 2023

I pushed in here by mistake, but how could I even do it :o

Feel free to force push on my last commit, I have it backed up.

@mariocynicys
Copy link
Collaborator

mariocynicys commented Jul 19, 2023

I did some tweaks in 2ad1d4c to use the default data directory in /root, omit making new user to run the tower from, avoid installing bash (not needed actually).

This is the command I run to boot up a dockered teos daemon:

docker run --network host --name teos -v ./datadir:/root/.teos --rm -it teos
## I set all the config options in `datadir/teos.toml`

Notice the -v option to persist the container's datadir in the host FS.

One could use -v with their already populated datadir and have the tower running just fine. but will need to tweak some config settings (shut tor off).

docker/README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
docker/README.md Outdated Show resolved Hide resolved
docker/README.md Show resolved Hide resolved
docker/README.md Outdated Show resolved Hide resolved
docker/README.md Outdated Show resolved Hide resolved
docker/README.md Outdated Show resolved Hide resolved
docker/README.md Show resolved Hide resolved
docker/README.md Outdated Show resolved Hide resolved
docker/entrypoint.sh Show resolved Hide resolved
@sr-gi
Copy link
Member

sr-gi commented Jul 19, 2023

I pushed in here by mistake, but how could I even do it :o

Feel free to force push on my last commit, I have it backed up.

You can allow others to add commits to your PR if they push to your repo. How did you push to her fork instead of yours, that I don't know lol

@orbitalturtle
Copy link
Contributor Author

orbitalturtle commented Jul 20, 2023

Thanks for the reviews! Those changes you made in the Dockerfile @mariocynicys seem good to me. Though I disagree with changing it to run as root - I think we should stick with the principle of least privilege, and I believe that's standard practice https://medium.com/@mccode/processes-in-containers-should-not-run-as-root-2feae3f0df3b

I'll finish making the above changes tomorrow ~

@mariocynicys
Copy link
Collaborator

Though I disagree with changing it to run as root - I think we should stick with the principle of least privilege, and I believe that's standard practice medium.com/@mccode/processes-in-containers-should-not-run-as-root-2feae3f0df3b

Very informative read. I didn't know that a root inside the container is just like a root process in the host :o. Just tried to use one of the protected ports 0-1023 for the tower and it worked just fine with it being root. This is definitely very scary.

@mariocynicys
Copy link
Collaborator

You can allow others to add commits to your PR if they push to your repo. How did you push to her fork instead of yours, that I don't know lol

I add new remotes to review then checkout the branch locally to push it to my remote instead, forgot to do the last part 😂 😂

@tsjk
Copy link

tsjk commented Jul 20, 2023

Why does this need --network=host?

@mariocynicys
Copy link
Collaborator

Why does this need --network=host?

running directly on the host network allows us to access bitcoind using localhost and allows us to listen for requests on the host port without the need to publish them.

I think we should actually go the Windows & OSX way for linux as well, to:

  • Unify how the image is ran over different platforms.
  • Limit the root access by not allowing the container to use the host network. (I am kinda convinced now that we can keep running as root user if we have no access to the host network nor the host FS).

@sr-gi
Copy link
Member

sr-gi commented Jul 24, 2023

Why does this need --network=host?

This is pretty old, but IIRC it was mainly for convenience. It was easier to setup. The MacOS/Windows section was added later in case someone wanted to do it for those platforms, but we expected most people to be running this in UNIX.

@orbitalturtle
Copy link
Contributor Author

I finally addressed your comments @mariocynicys and @sr-gi, when you have time for another look. :)

And yes, the tutorial right now assumes you're running Teos in Docker on the same machine as bitcoind. As I eventually get further along with my own Teos Docker setup I'll try to add a guide for someone who might be running bitcoind on a separate raspberry pi, or something, which seems like the more likely real-world scenario.

Copy link
Member

@sr-gi sr-gi left a comment

Choose a reason for hiding this comment

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

Mainly nits about structure/naming conventions.

I normally use teosd when referring to only the tower backend. teos when talking about the project or the system as a whole. I never capitalize this btw, so no Rust-teos, Teos, Teosd, ...

It may be good to test whether you're able to connect a CLN client to the tower running on docker, given the issue I was facing with RPC. This may only apply to macOS/Windows though.

docker/Dockerfile Outdated Show resolved Hide resolved
docker/Dockerfile Outdated Show resolved Hide resolved
docker/Dockerfile Outdated Show resolved Hide resolved
docker/Dockerfile Outdated Show resolved Hide resolved
docker/Dockerfile Outdated Show resolved Hide resolved
docker/README.md Outdated Show resolved Hide resolved
docker/README.md Outdated Show resolved Hide resolved
docker/README.md Outdated Show resolved Hide resolved
docker/README.md Outdated Show resolved Hide resolved
docker/entrypoint.sh Outdated Show resolved Hide resolved
@sr-gi sr-gi mentioned this pull request Jul 27, 2023
@orbitalturtle
Copy link
Contributor Author

orbitalturtle commented Jul 28, 2023

@sr-gi Ahhh yup I now remember the lowercase teos naming conventions from the python teos days :p

I haven't tried connecting using the CLN client yet... But re: connecting to teosd from the host computer with teos-cli. It has been working for me without setting RPC_BIND, in linux anyways. Once teosd is running in docker, it should be exposed on localhost from the host. Then I needed to copy the needed tls cert data from the docker /.teos folder to the host's /.teos folder, so that teos-cli could connect w/o authentication issues.

What error are you getting? Just not able to connect? When using docker run, do you have the -p 9814:9814 field set?

I'll give it a try on non-linux OSs in VirtualBox some day soon to see if I can come up with anything that'll help

(Also, made those other changes that you requested above)

@orbitalturtle
Copy link
Contributor Author

@sr-gi Just confirmed that loading the plugin in CLN and using lightning-cli registertowe to connect to the tower running in Docker works for me too

docker/README.md Outdated
- BTC_RPC_PORT=<btc_node_port>
- BTC_RPC_USER=<btc_rpc_username>
- BTC_RPC_PASSWORD=<btc_rpc_password>
- DATA_DIR=<teos_data_dir>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's hard code this (or leave the default value) in entrypoint.sh.
data_dir is the data directory inside the container, which shouldn't be relevant to the user and changing it would require changing the mount mapping.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call, just removed that option 👍🏻

docker/README.md Show resolved Hide resolved
docker/Dockerfile Outdated Show resolved Hide resolved
docker/Dockerfile Outdated Show resolved Hide resolved
@mariocynicys
Copy link
Collaborator

Was able to use a non-root user with user-provided data directory: mariocynicys@d2b1f31

Basically, this change chowns the data directory in the container runtime and not the image build time.

@sr-gi
Copy link
Member

sr-gi commented Aug 4, 2023

I need to re-check this on MacOS to see if I'm able to use the CLI/Client from outside

Copy link
Member

@sr-gi sr-gi left a comment

Choose a reason for hiding this comment

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

I've been testing this and I was finally able to make it work on MacOS. I think the minimal that is needed is:

docker run \
  -p 9814:9814 \
  -p 8814:8814 \
  --name teos \
  --mount source=teos-data,target=/root/.teos \
  -e BTC_RPC_CONNECT=host.docker.internal \
  -e BTC_RPC_USER=<btc_node_hostname> \
  -e BTC_RPC_PASSWORD=<btc_rpc_password>\
  -e API_BIND=0.0.0.0 \
  -e RPC_BIND=0.0.0.0 \
  -it teos

This will enable bidirectional communication between the host and the container. However, teos-cli may not be usable, giving the credentials may not match.

Also, I've been able to run the Tor bit and interact with a lightning client over Tor

docker/Dockerfile Outdated Show resolved Hide resolved
docker/README.md Outdated Show resolved Hide resolved
docker/README.md Outdated Show resolved Hide resolved
docker/README.md Outdated Show resolved Hide resolved

`docker exec -it <CONTAINER NAME> sh`

Then you can use the `teos-cli` binary from inside the container as you would use it from your host machine.
Copy link
Member

Choose a reason for hiding this comment

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

Not sure this would really work

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm can you elaborate? Tested and it seems to work for me.

Copy link
Member

Choose a reason for hiding this comment

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

Can you run teos-cli from the host pointing to the container? That should raise an authentication issue, given the credentials of your .teos outside the container are different to the ones inside

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So option 1 is running within the Docker instance so there shouldn't be an authentication issue. But yes, if running from the host machine, then you have to use option 2 (below)

docker/README.md Outdated Show resolved Hide resolved
docker/README.md Outdated Show resolved Hide resolved
@orbitalturtle
Copy link
Contributor Author

Thanks again for the reviews y'all. This is definitely looking more battle tested. I addressed those comments... I think the top portion of the README had the most changes since I reorganized it to add an option to copy over the config to Docker, upon feedback from @mariocynicys.

@sr-gi Awesome, glad to hear you got it working on MacOS. I updated the instructions with that new command, if that seems like a good enough fix.

Hoping it's close! 🤞🏻

docker/README.md Outdated Show resolved Hide resolved
docker/README.md Outdated
--network=host
--name teos \
--mount source=teos-data,target=/home/teos/.teos \
-e BTC_RPC_CONNECT=host.docker.internal \
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this should be needed for Linux, given we are using --nerwork=host. Can you confirm?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes you're correct 👍🏻


`docker exec -it <CONTAINER NAME> sh`

Then you can use the `teos-cli` binary from inside the container as you would use it from your host machine.
Copy link
Member

Choose a reason for hiding this comment

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

Can you run teos-cli from the host pointing to the container? That should raise an authentication issue, given the credentials of your .teos outside the container are different to the ones inside

Copy link
Member

@sr-gi sr-gi left a comment

Choose a reason for hiding this comment

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

LGTM

docker/Dockerfile Outdated Show resolved Hide resolved
docker/Dockerfile Show resolved Hide resolved
docker/Dockerfile Outdated Show resolved Hide resolved
docker/Dockerfile Outdated Show resolved Hide resolved
docker/Dockerfile Show resolved Hide resolved
docker/README.md Outdated
Comment on lines 39 to 42
If you'd like to use a `teos.toml` file instead, one option is to edit the Dockerfile with this line right above the ENTRYPOINT:

COPY teos.toml /home/teos/.teos
Copy link
Collaborator

Choose a reason for hiding this comment

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

Better instruct the usage of docker cp instead to avoid rebuilding the image.
Also, the whole data directory gets cleared if a volume is attached to while running container.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't realize this before, but docker cp won't actually work here because it can only be used once the container is running, but we need to copy over this data before it's running and we hit the endpoint.

Can you elaborate on what you mean by the second point?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I didn't realize this before, but docker cp won't actually work here because it can only be used once the container is running, but we need to copy over this data before it's running and we hit the endpoint.

The way I did it (but think it's tedious tbh) is spin up a dummy alpine container with the volume just to copy the data over to the volume with docker cp, and then reuse the volume with teos image.

Can you elaborate on what you mean by the second point?

When a volume is attached, any image data inside the mount point disappears, I guess what docker does is just replace the directory from the image with the volume content. So the COPY command provides some data at build time, which if you run the container without a volume would persist as you would expect, but if a volume is attached, the mount directory would be identical to what's inside the volume, effectively ignoring the build-time COPY command.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh got it, interesting. Well one option in that case would be to add a VOLUME command above the COPY command in the Dockerfile to create the volume before rather than after

Personally, I don't think we don't need to get super deep in these instructions into... what if a user does this, what will happen? It's nice that we have some instructions to give users an outline, but I suspect most people using this Dockerfile will have some knowledge of Docker and will know how to tweak it for their specific needs

Copy link
Collaborator

Choose a reason for hiding this comment

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

Fine, let's remove the config file instructions and maybe just mention that such a method exists and leave it up to the user how to do so.

docker/README.md Outdated Show resolved Hide resolved
docker/README.md Outdated Show resolved Hide resolved
docker/README.md Show resolved Hide resolved
docker/entrypoint.sh Outdated Show resolved Hide resolved
Copy link
Collaborator

@mariocynicys mariocynicys left a comment

Choose a reason for hiding this comment

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

LGTM :)

@orbitalturtle
Copy link
Contributor Author

@mariocynicys Yay! Made that last remaining change.

@sr-gi
Copy link
Member

sr-gi commented Sep 14, 2023

Re-ACK c3c7e99

@mariocynicys
Copy link
Collaborator

ACK c3c7e99

@sr-gi
Copy link
Member

sr-gi commented Sep 14, 2023

Needs rebase

@orbitalturtle
Copy link
Contributor Author

@sr-gi @mariocynicys just rebased

@sr-gi sr-gi merged commit a4acced into talaia-labs:master Sep 14, 2023
7 checks passed
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.

4 participants