-
-
Notifications
You must be signed in to change notification settings - Fork 33
Add installation using Docker Compose and many other improvements for implementation in Docker. #614
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
base: main
Are you sure you want to change the base?
Conversation
- Add markdown tabs blocks - Fix [Issue 604](chatmail#604) - Add `--skip-dns-check` argument to `cmdeploy run` command - Add `--force` argument to `cmdeploy init` command - Add startup for `fcgiwrap.service` - Add extended check when installing `unbound.service` - Add configuration parameters (`is_development_instance`, `use_foreign_cert_manager`, `acme_email`, `change_kernel_settings`, `fs_inotify_max_user_instances_and_watchers`)
great work and offer of contribution, @Keonik1 -- the silence since friday is more accidental but several people are excited about this PR and want to try it out and feedback. Not doing that myself right now but wanted to let you know :) |
I am very interested in deploying with containers as well. In particular, I would like to decouple from systemd and the host operating system, so that the software content of the components can be managed independently. Doing that will require an understanding of what all the components are, and toward that end, I created #615 to document what I know so far. I like the re-use here of the pyinfra-based script to populate the container; I think sharing the process as much as possible between containers and host-based deploys is a good idea. I think with some revisions the process could use pyinfra's |
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.
Soo many changes :) thanks for all the effort and thoughts that went into this.
Sorry for taking some days to reply, but I wanted to look at it in full detail, and have some fix suggestions already. I got acmetool to work, for example.
The tests are mostly fine with this PR, but unfortunately many tests try to login via SSH - maybe we can try to run docker exec
if a chatmail container is running on the same machine?
A github action which deploys the docker container automatically would also be nice :) We can use a hetzner VPS for that, like for the cmdeploy CI. It would be good to have CI notice when something fails with a new change, to avoid regressions.
In general, not bad how many variables and configuration options you added, e.g. that CHANGE_KERNEL_SETTINGS
is set to False by default in setup_chatmail_docker
. In most situations the defaults will probably never be overridden, but it's good that it's explicit what's the behavior.
The markdown tabs are also a nice touch, but we might need more discussion to merge them into main. The question is, which languages do you prefer, which do you offer? Only offering english is not much better, sure. It requires some thought, I think, or could go to an example page so admins can choose to have the tabs if they want to be multi-lingual. Right now, many admins just translate the www directory to their mother tongue.
All in all, thanks for all of this! I suggest to address the comments, and merge it to a docker
branch for now, so we can make it more stable in subsequent PRs. But in any case there will be some interested testers already :)))
cmdeploy/src/cmdeploy/cmdeploy.py
Outdated
def get_sshexec(): | ||
print(f"[ssh] login to {args.config.mail_domain}") | ||
return SSHExec(args.config.mail_domain, verbose=args.verbose) | ||
host = args.ssh_host if hasattr(args, "ssh_host") and args.ssh_host else args.config.mail_domain | ||
print(f"[ssh] login to {host}") | ||
return SSHExec(host, verbose=args.verbose) |
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 is a good step towards getting cmdeploy dns
to run as well... but maybe we can completely avoid logging in to localhost? That would be much simpler. Let's see, for now at least deploying works, after all :)
@missytake, Hello! I've also thought about it and will try to send fixes in the format of |
i have 2 ideas:
It's also probably worth adding the ability to determine which languages will be displayed in the installed instance, so that admins can simply enable or disable if necessary. I'll think about the implementation of this at the end, when I fix everything else. |
if sshexec == "localhost": | ||
cmd = f"{pyinf} @local {deploy_path} -y" |
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 is a good quick fix, maybe we can add something like this to the cmdeploy dns
calls as well, so they don't try to request DNS records from the chatmail relay itself, but from the local (docker) host.
We're getting there! With the last comments addressed, I suggest to merge it into main, but maybe don't mention it in README.md for now (or only as experimental option). But @hpk42 and @link2xt should also add their opinion on this. To summarize what's needed in the long run:
|
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 can hardcode this variable, as it is only used in one place anyway :)
MAIL_DOMAIN="chat.example.com" | ||
ACME_EMAIL="[email protected]" | ||
|
||
CERTS_ROOT_DIR_HOST="./traefik/data/letsencrypt/certs" |
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 variable is a bit confusing if you use the non-traefik-setup... and without traefik, it is actually not used, right?
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 can be used not only for installation with Traefik, but also for any third-party certificate manager. Traefik is simply more convenient in this regard and is a good example.
I think you can write a comment directly in the example.env file stating that if you are using the standard installation, you can leave this field untouched, as it will not have any effect.
Or we can remove it altogether if we want a more linear installation.
## system | ||
- /sys/fs/cgroup:/sys/fs/cgroup:rw # required for systemd | ||
- ./:/opt/chatmail | ||
- ${CERTS_ROOT_DIR_HOST}:${CERTS_ROOT_DIR_CONTAINER}:ro |
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.
- ${CERTS_ROOT_DIR_HOST}:${CERTS_ROOT_DIR_CONTAINER}:ro | |
- ./traefik/data/letsencrypt/certs:${CERTS_ROOT_DIR_CONTAINER}:ro |
Then we could also just hardcode the value here.
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.
Have not read to the end yet.
params.get("change_kernel_settings", "true").lower() == "true" | ||
) | ||
self.fs_inotify_max_user_instances_and_watchers = int( | ||
params["fs_inotify_max_user_instances_and_watchers"] |
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 looks like it will fail if the setting is not in the config file.
out.green(f"created config file for {mail_domain} in {args.inipath}") | ||
if not args.recreate_ini: | ||
out.green(f"[WARNING] Path exists, not modifying: {inipath}") | ||
return 1 ### need research - can we set return code as zero? |
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 can because nobody uses it, but why? Running init
when it does nothing is likely not what you want.
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.
Well, this is probably an architectural issue. I think that calling init again shouldn't cause an error.
@hpk42 @missytake
What do you think about this?
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 agree with @link2xt that if re-creating the chatmail.ini file is not wished for, an existing chatmail.ini is an error; your command will not have the effect you want it to have (creating a default configuration file for a given domain).
Scripts which want to re-create the ini like setup_chatmail_docker.sh
can simply pass --force
or || true
here, depending on the desired behavior.
"--skip-dns-check", | ||
dest="dns_check_disabled", | ||
action="store_true", | ||
help="disable checks nslookup for dns", |
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.
help="disable checks nslookup for dns", | |
help="disable nslookup checks for DNS", |
if [ "${USE_FOREIGN_CERT_MANAGER,,}" == "true" ]; then | ||
if [ ! -f "$PATH_TO_SSL/fullchain" ]; then | ||
echo "Error: file '$PATH_TO_SSL/fullchain' does not exist. Exiting..." > /dev/stderr | ||
sleep 2 |
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 is the reason for sleep
here? Is it a workaround for 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.
@link2xt because it will restart ~5 containers per second, but we need 2-10 seconds to obtain certificates
|
||
unlink /etc/nginx/sites-enabled/default || true | ||
|
||
if [ "${USE_FOREIGN_CERT_MANAGER,,}" == "true" ]; then |
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.
if [ "${USE_FOREIGN_CERT_MANAGER,,}" == "true" ]; then | |
if [ "${USE_FOREIGN_CERT_MANAGER,,}" = true ]; then |
=
is standard, ==
is a bash extension for pattern matching that is not needed here.
fi | ||
fi | ||
|
||
SETUP_CHATMAIL_SERVICE_PATH="${SETUP_CHATMAIL_SERVICE_PATH:-/lib/systemd/system/setup_chatmail.service}" |
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.
SETUP_CHATMAIL_SERVICE_PATH="${SETUP_CHATMAIL_SERVICE_PATH:-/lib/systemd/system/setup_chatmail.service}" | |
: "${SETUP_CHATMAIL_SERVICE_PATH:=/lib/systemd/system/setup_chatmail.service}" |
Same result, just shorter.
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.
True, but also harder to read for people without an extensive bash background^^ 🤷
SETUP_CHATMAIL_SERVICE_PATH="${SETUP_CHATMAIL_SERVICE_PATH:-/lib/systemd/system/setup_chatmail.service}" | ||
|
||
env_vars=$(printenv | cut -d= -f1 | xargs) | ||
sed -i "s|<envs_list>|$env_vars|g" $SETUP_CHATMAIL_SERVICE_PATH |
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.
Do we actually need to pass all the environment variables? This is not easy to understand without reading the setup_chatmail.service
template. At least needs a comment to explain what happens here.
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.
@link2xt
systemd starts services without passing environment variables to them. A running service can only use the environment variables specified in the some.service
file.
Hi, when I decided to set up a chatmail server for personal use, I was frustrated to discover that there was no Docker installation, so I created a preliminary implementation.
Brief description of changes
--skip-dns-check
argument tocmdeploy run
command--force
argument tocmdeploy init
commandfcgiwrap.service
unbound.service
is_development_instance
,use_foreign_cert_manager
,acme_email
,change_kernel_settings
,fs_inotify_max_user_instances_and_watchers
)Full description of changes
The main thing that was done was to add support for running in Docker, but in addition to that, the problems that made this method of running impossible were also solved.
I think the advantages of running in Docker are obvious:
This is only the first implementation of installation using Docker, but it works and seems to be stable (at least I haven't found any anomalies), but in the future it will need to be brought to a more adequate state (with the abandonment of using systemd inside and the division of everything into several containers or 1 container, but without systemd management, but that's just a thought for the future).
The modifications were made with a focus on full compatibility with the current installation and are additional optional parameters.
In addition, some QoL features and instance documentation unification were added so that administrators would not have to create separate pages for different language groups of users.
Some errors that I encountered during deployment have also been fixed (and to be honest, I don't understand why some of them didn't occur for others -_-), namely:
--ssh_host
argumentfcgiwrap.service
service, which does not download or start when rolled out using the current methodunbound.service
, when it is installed but does not show that it occupies port 53All changes are described in detail in the
changelog.md
.I hope my improvements meet your requirements and you accept the PR, because I really believe that this is a very important part of the project's development — installation using Docker is easier, which means that many more people will be able to install it and thus popularize it.