Usable Dockerfile for CLightning

[NicolasDorier]

I already did a PR in the past with this, but decided to close it after there was complains about socat dependency.

This PR makes a second attempt, this time, socat is disabled by default. (it is used only for dev purposes)
It ships litecoin-cli and bitcoin-cli with it.

Environment variable:

• EXPOSE_TCP default to false, if true, use socat to expose clightning on port 9835.
• LIGHTNINGD_OPT is the config file of the clightning instance.

A new parameter, not existing in clightning parameters called chain=btc|ltc has been added and is interpreted in the entrypoint.sh, it will transform the network= parameter with the following table if present:

chain + network = replaced network

btc + mainnet = bitcoin
btc + testnet = testnet
btc + regtest = regtest
ltc + mainnet = litecoin
ltc + testnet = litecoin-testnet

This has been done to uniformise the way of identifying the chain to use across different docker images I am using.

Note that this image is used in production by BTCPay (https://hub.docker.com/r/nicolasdorier/clightning/), so I don't strictly need it in the lightning repository. But I think it has better its place here than in my fork. (I rebase this PR/docker image one time per week)

Using EXPOSE_TCP allow the contributors to debug BTCPay with clightning integration whatever the OS they are using.

[practicalswift]

Nit: Missing \n

[practicalswift]

Nit: Inconsistent indentation.

[practicalswift]

Nit: Missing \n

[practicalswift]

Please fix the linting warnings reported by shellcheck tools/docker-entrypoint.sh :-)

[NicolasDorier]

Thanks, I fixed the indent (will squash later), for the shellcheck, will do after once there is some Concept ACK. (Developing on windows, so I need some time to set it up)

[practicalswift]

Concept ACK from me :-)

[NicolasDorier]

Cool! Ping @cdecker does it change your mind since #1187 ?

[cdecker]

Thanks @NicolasDorier this is indeed much better 😃 Making socat optional is much better I think.

I'd prefer to have these files in the contrib directory though, since that is where we stash most of the auxiliary files. That makes COPY . . harder, but then again I'd prefer having the builder image just checkout from git which should guarantee relative stability of the compiled binary. Alternatively we can write a Makefile that tars up the files from git ls-files and copies that tarball in.

[NicolasDorier]

This docker is meant to be integrated with docker hub automated build. This mean that everytimes you commit, dockerhub pick it up, build from the sources and apply a tag to it.

Using COPY . ., you are sure that the automated build is building an image which is the same as the sources of the commit. It makes it also easy for someone to tweak clightning code then build themselves their own images and test again. (no need to git push / tweak the dockerfile before doing a docker build)

[analogic]

Would not be better to use debian:stretch-slim? System itself it is same as ubuntu but resulting image will be almost 30MB smaller...

[NicolasDorier]

I am not against it, just not a linux expert and I don't really have time to hunt about which package name / tooling change between distro. :(

[analogic]

Technicaly Ubuntu extends Debian, tooling and packages are mostly same. This will do the trick (I've tested build):

1c1
< FROM ubuntu:16.04 as builder
---
> FROM debian:stretch-slim as builder
4c4
<   libsqlite3-dev python python3 wget
---
>   libsqlite3-dev python python3 wget gnupg dirmngr
42c42
< FROM ubuntu:16.04
---
> FROM debian:stretch-slim

[NicolasDorier]

Thanks @analogic , trying and testing this now.

[NicolasDorier]

Fixed spellcheck (travis was quite useful), added and tested with debian, it works fine.

[practicalswift]

@NicolasDorier Do you mean shellcheck? :-)

[NicolasDorier]

@cdecker if you concept ACK the COPY . . (which is unavoidable if you want to leverage automated build on docker hub), I'll work on adding a good documentation in the README about this.

[cdecker]

Yeah, I see your point, just a bit hesitant to put stuff in the root of the project :-)

Concept ACK

Edit: regarding docker image size, we might even get away with alpine, which is truly tiny.

[analogic]

@cdecker Sure, Alpine would work too, but I am afraid that it is will make image harder to extend for average docker user

[NicolasDorier]

Just added documentation in the README.md to teach how to use this docker file.

[NicolasDorier]

Note that the doc assumes you have setup automated build on docker hub on elementsproject/lightning !

[practicalswift]

Nit: Missing \n

[practicalswift]

Nit: Remove redundant \n

[cdecker]

elementsproject/lightning -> elementsproject/lightningd

[NicolasDorier]

@jsarenik the DEVELOPER=1 should be set also by the runtime image or is this just a build time variable?

Knowing that the runtime image is different from the build image, the config.vars whatever it is, is not copied in the runtime image.

[NicolasDorier]

mmh getting external/libsodium is not a submodule!

git submodule init && git submodule update did not help.

UPDATE: debugging out, might come from CRLF ;S

[jsarenik]

@NicolasDorier just once and just during build. Either by exporting DEVELOPER=1 in environment before running ./configure OR running configure with --enable-developer.

[cdecker]

I'm currently setting things up, but we might end up having to manually push them for the time being (security settings around this org is pretty strict).

Also I find myself overriding the --entrypoint in almost every run, so I wonder if the entrypoint script isn't doing too much for its own good. If we go down this route we'll have two different ways to invoke the daemon (through the config and through command line args) that do not match. In particular I find it annoying that command line args get ignored.

Can we trim the magic down to adding prefix and suffix to the executed command, but not completely overriding? I like the tracing instrumentation, but I don't like the forced path through a config file and the cli args being discarded.

[jsarenik]

By the way I have good experience with separate set of scripts inside the git tree which are shared across all CI systems (e.g. Travis config would point to a script that contains commands instead of defining them, the same for Docker, etc.)

See https://github.com/jsarenik/spf-tools/tree/master/misc and https://github.com/jsarenik/spf-tools/blob/master/.travis.yml

[NicolasDorier]

Why are you overriding with --entrypoint ? It never happened once to me

[cdecker]

Because I'd like to have access to the other command line flags (like --log-level, --lightning-dir, ...) without having to edit a config file.

[shesek]

Because I'd like to have access to the other command line flags without having to edit a config file.

Lightning Charge allows specifying an LN_OPT environment variable that gets passed as arguments to lightningd, which you can then specify with i.e. docker run -e LN_OPT='--log-level debug' elements/lightning.

Alternatively, since lightningd is the main thing being run here, the entrypoint can forward $@ instead then specify with i.e. docker run elements/lightning --log-level debug, which is even nicer.

[NicolasDorier]

@cdecker why not doing

docker run -e LIGHTNINGD_OPT=" \
log-level=debug
blah=value
" clightning/lightningd 

But additionally, I can also pass the pass parameters so you can do this:

docker run clightning/lightningd --log-level=debug blah=value

UPDATE: just saw @shesek response. But indeed it would be the same approach as charge which work well and is flexible enough.

[NicolasDorier]

@cdecker I pushed, try docker run clightning/lightningd --log-level=debug it should work as well.

[shesek]

Note that network is still expected to be specified only via LIGHTNINGD_OPT, meaning that docker run clightning/lightningd --network mainnet won't work as expected, which might be somewhat confusing.

Perhaps it would be better to specify the network and chain using separate environment variables, instead of overloading network in the config file?

Also, mutating the config file, which might be mounted from outside, doesn't seem like the cleanest solution. Using environment variables and passing the resulting "REPLACEDNETWORK" as an argument to lightningd, rather than via the config file, seems better.

Edit: and the fact that lightningd and lightningd's docker treat "network" as having a different meaning is a sure source of confusion... how about using the CHAIN environment variable with values like BTC-testnet instead?

[NicolasDorier]

Also, mutating the config file, which might be mounted from outside, doesn't seem like the cleanest solution.

I never had any reason to mount and modify the config file outside the docker-compose or command line parameters via mounted folders. But what we can do in this case is to not modify the config file if LIGHTNINGD_OPT is blank.

I think supporting the REPLACEDNETWORK feature should be done only in config file. Because if we plan to support via command line as well, we need even more complex logic to handle whether chain and network comes from config/command line or both and also rewriting the command line arguments... Lot's of headaches not worth the additional code.

The best would be either, we keep only for config file, or to back this directly into clightning code, or that we completely remove it.
However, if you remove it, I'll still need to maintain my own fork which use this for my projects instead of using clightning's repo version. :(

[shesek]

Because if we plan to support via command line as well, we need even more complex logic to handle whether chain and network comes from config/command line or both and also rewriting the command line arguments

I would suggest that:

1. Don't do any special handling for the config file at all, let it simply pass through. (or even, just remove the file-related LIGHTNING_OPT handling entirely in favor of passing args)

2. Introduce separate environment variables configuration for chain/network, without calling them "network".

3. If that new config is passed, determine network based on them and append it to the arguments list. Otherwise, just pass the argument as-is.

That seems to be even simpler than the current implementation, no?

[NicolasDorier]

Ok so how about this which seem nice compromise:

1. Keep LIGHTNING_OPT
2. Adding LIGHTNING_CHAIN (btc, ltc) and LIGHTNING_ENVIRONMENT (mainnet,testnet,regtest).
3. If those are present, add --network in the list of command line parameters.
4. If LIGHTNING_OPT is null or empty, do not attempt to create or override the config file.

When using docker compose, passing the configuration file in the docker-compose file is more convenient and readable, so I am not too happy removing it.

[NicolasDorier]

Closing this one, supersede by #1616