Skip to content

Rework docker images build #10505

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

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Rework docker images build #10505

wants to merge 7 commits into from

Conversation

yhabteab
Copy link
Member

This PR is similar to the changes made in the Icinga DB repository. This allows for more flexibility and consistency in building processes of the docker images as opposed to the previous method from the docker-icinga2 repository. The previously used Dockerfile has been renamed to Containerfile and the build process has been updated accordingly to make use of the Docker BuildKit caching capabilities. This change is expected to improve the build performance and allows for better layer caching, which drastically reduces the build time for subsequent builds on local machines.

As opposed to the previous behaviour, the current build process doesn't require a path to the source code to be passed
to the docker build command. Instead, the source code is bind mounted into the container at build time, more importantly
it doesn't require you to commit any changes made locally, but it simply uses the current state of the source code directory.
It's mounted as a readonly into the container, so no changes can be made to the source code from within the container.

Apart from that, this PR also automatically fixes various issues from the previous repository, such as:

  • There's now no intermediate locations for the config files, they are placed directly into the /data directory at build time. This eliminates the need for subsequent initialization steps by the container entrypoint script to copy the files around. As a result, the container entrypoint script has been dramatically simplified and now only contains the necessary steps to run a icinga2 node setup command if needed. This also means that users can now simply mount their own configuration files into the /data without any issues.
  • Since most of the issues in the legacy repository are about mounting issues or missing necessary env variables to additionally configure the container, this PR effectively removes the need for most of them. The only required environment variables are the ones that are already supported and documented in the docker-icinga2 repository. If a user wants to configure the container further, they can simply mount their own configuration files into the /data directory.

This PR also includes a For-Container.md file that contains the necessary information for users to set up the container and use it effectively. It provides a comprehensive guide on how to run the container, including the necessary environment variables, and how to mount configuration files. This file is intended to be a replacement for the README.md file in the docker-icinga2 repository.

Note: Commits from 5cd9ab2...20f28ec are cherry-picked from the Icinga DB repository.

See pushed images on Docker Hub.
See pushed images on GHCR.

@cla-bot cla-bot bot added the cla/signed label Jul 21, 2025
@yhabteab yhabteab requested review from julianbrost and lippserd July 21, 2025 10:54
@yhabteab yhabteab added enhancement New feature or request area/ci CI/CD labels Jul 21, 2025
Copy link
Member

@Al2Klimov Al2Klimov left a comment

Choose a reason for hiding this comment

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

It doesn't work with Docker Desktop on M3:

➜  icinga2 git:(docker-v2) docker run --rm -itv my-new-vol-4:/data -e ICINGA_MASTER=1 docker.io/icinga/icinga2:test
[2025-07-22 09:34:31] information/DockerEntrypoint: Icinga 2 Docker entrypoint script started.
[2025-07-22 09:34:31] information/DockerEntrypoint: Running Icinga 2 node setup command...
information/cli: Checking in existing certificates for common name '41c044b13034'...
information/cli: Certificates not yet generated. Running 'api setup' now.
information/cli: Generating new CA.
critical/Application: Error: Function call 'mkdir' for file '/data/var/lib/icinga2/ca' failed with error code 13, 'Permission denied'


Additional information is available in '/data/var/log/icinga2/crash/report.1753176871.799910'
/usr/local/bin/docker-entrypoint.sh: line 115:    16 Aborted                 icinga2 "${nodeSetup[@]}"
➜  icinga2 git:(docker-v2)

@yhabteab
Copy link
Member Author

It doesn't work with Docker Desktop on M3:

Is not architecture specific :) but thanks anyway for testing! I was thinking /usr/lib/icinga2/prepare-dirs would actually fix all Icinga 2 related directories but not its only purpose is to fix the /var/run, /var/cache etc. directories. With the last commit it should work normally now.

@Al2Klimov Al2Klimov dismissed their stale review July 22, 2025 13:00

OP addressed review

@yhabteab yhabteab force-pushed the docker-v2 branch 2 times, most recently from d9ab97d to 1b7aace Compare July 28, 2025 11:19
@yhabteab yhabteab force-pushed the docker-v2 branch 2 times, most recently from f49b776 to 520bdba Compare July 29, 2025 09:01
@yhabteab
Copy link
Member Author

Sorry, for the force pushes! Just wanted to test the concurrency rules! I won't push anything now unless someone says otherwise.

@yhabteab yhabteab force-pushed the docker-v2 branch 2 times, most recently from 928c5a3 to ae6aab0 Compare July 30, 2025 09:58
@yhabteab yhabteab requested a review from julianbrost July 30, 2025 10:02
system if needed. The following command will start an Icinga 2 master container with the necessary configurations:

```
docker run --rm --detach \
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this (and other commands in this file) really pass --rm? I mean the data is safe inside the volume, but deleting the container when is stops still doesn't sound right.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm just using the exact same commands used in the original README.md file.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think I start to see a pattern here 🙈

Containerfile Outdated
--home /var/lib/icinga2 \
--disabled-login \
--no-create-home \
--uid 5665 icinga
Copy link
Contributor

Choose a reason for hiding this comment

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

I really dislike this UID=5665 thing. It isn't your fault obviously, since it was the same in the old docker-icinga, but it makes bind-mounting local files/directories into the containers needlessly hard for what's essentially an easter-egg. Maybe the icinga user id could also be made an ARG defaulting to this silly value of 5665 for backwards-compatibility and people that use the defaults and don't care.

Copy link
Member Author

Choose a reason for hiding this comment

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

but it makes bind-mounting local files/directories into the containers needlessly hard for what's essentially an easter-egg.

Can you explain what problems you’re exactly facing? Why should the user id cause problems with bind mounts? Ah, I didn't just let this as-is just because it was so in the legacy docker file but due to this note in the docker best practices and the value for it was already chosen, so I just used it here.

Copy link
Contributor

Choose a reason for hiding this comment

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

and the value for it was already chosen, so I just used it here.

That's what I meant. Setting a defined value is great. I would want it to be set to a defined 1000, because that's the UID of my own user, which means I can for example painlessly bind-mount the certs into the containers in a way they're writable for the icinga daemon so it can renew them automatically or lets me easily issue arbitrary new ones. That's just an example though, having an rw bind mount between host and container can be useful in a number of ways and doesn't cost us anything except an additional ARG.

Copy link
Contributor

Choose a reason for hiding this comment

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

Say that you aren't using a Linux machine without saying that explicitly 😅

On a single-user Linux machine, your user account typically has UID 1000. Container images often use UID 1000 internally, as that's the one that will be typically assigned if you don't specify it. With that combination (and if user namespaces aren't used), creating something outside of the container and then bind-mounting it will magically have the correct permissions for it to be accessible by the application inside the container, and vice versa for things created by the application inside the container.

Copy link
Member

Choose a reason for hiding this comment

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

Do we really need this as ARG? What about something like: docker run -v $(pwd):/data --user $(id -u):$(id -g) ...

Copy link
Contributor

@jschmidt-icinga jschmidt-icinga Jul 31, 2025

Choose a reason for hiding this comment

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

@lippserd That won't work with icinga2 which uses getpwnam() and getgrnam() to make sure the user/groupo it's running as actually exist on the system, which leads to the following error messages and termination:

master-1  | critical/cli: setgroups() failed with error code 1, "Operation not permitted"
master-1  | critical/cli: Please re-run this command as a privileged user or using the "icinga" account.

That's probably the reason why we're adding a user in the Containerfile in the first place. I'm not an expert in docker, but maybe we could instead adapt the entrypoint to add a user for its current uid instead of doing so in the build process?

@yhabteab yhabteab requested review from Al2Klimov, julianbrost and jschmidt-icinga and removed request for Al2Klimov July 30, 2025 13:12
yhabteab added 4 commits July 30, 2025 17:46
Previously, the https://github.com/Icinga/docker-icinga2 repository was
used to build the Docker images for Icinga 2. However, due to its
various design flaws, the resulted images had limited usability and
required a lot of manual tweaking to make something useful out of them.
This commit now follows our new principles of building Docker images
from the Icinga DB repository, and replaces the old separate repository
with this one. It makes use of the newest Docker BuildKit features to
build the images in a more efficient way, while also granting users full
flexibility to easily extend or modify the images as they see fit
without any issues.
Containerfile Outdated
Comment on lines 219 to 220
COPY tools/container/entrypoint.sh /usr/local/bin/entrypoint.sh
RUN chmod +x /usr/local/bin/entrypoint.sh
Copy link
Contributor

Choose a reason for hiding this comment

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

COPY "leaks" the permissions from the source directory into the container. So chmod +x isn't really necessary here as the file is already marked as executable in Git. However, Git only tracks executable yes/no, no user/group/other permissions. So if you run the Git operations with a more restrictive umask, this results in a broken container:

$ umask 077
$ git clone -b docker-v2 https://github.com/Icinga/icinga2.git
$ cd icinga2
$ docker build --tag icinga/icinga2:test --file Containerfile .
$ docker run --rm -it icinga/icinga2:test
/bin/bash: /usr/local/bin/entrypoint.sh: Permission denied

That happens because in the source directory, the file has mode 700 and chmod +x changes that to 711.

Suggested change
COPY tools/container/entrypoint.sh /usr/local/bin/entrypoint.sh
RUN chmod +x /usr/local/bin/entrypoint.sh
COPY tools/container/entrypoint.sh /usr/local/bin/entrypoint.sh
RUN chmod 0755 /usr/local/bin/entrypoint.sh

Containerfile Outdated
-DCMAKE_INSTALL_LOCALSTATEDIR=/data/var \
-DICINGA2_SYSCONFIGFILE=/etc/sysconfig/icinga2 \
-DICINGA2_RUNDIR=/run \
-DICINGA2_WITH_{COMPAT,LIVESTATUS}=OFF && \
Copy link
Contributor

Choose a reason for hiding this comment

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

If the other uses of {...,...} need replacement, this one probably does too:

Suggested change
-DICINGA2_WITH_{COMPAT,LIVESTATUS}=OFF && \
-DICINGA2_WITH_COMPAT=OFF \
-DICINGA2_WITH_LIVESTATUS=OFF && \

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/ci CI/CD cla/signed enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants