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

tests: Initial Integration Tests #144

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Conversation

oxzi
Copy link
Member

@oxzi oxzi commented Dec 20, 2023

Heavily inspired by the already existing integration tests from Icinga DB, the icinga-testing project was extended to support Icinga Notifications: Icinga/icinga-testing#26, Icinga/icinga-testing#27

Based on those changes, an initial integration test was implemented for Icinga Notifications, currently just launching PostgreSQL, Icinga 2, and Icinga Notifications, waiting for Icinga Notifications to scan the available channels and write them into the database.

@oxzi oxzi marked this pull request as draft December 20, 2023 14:46
@cla-bot cla-bot bot added the cla/signed CLA is signed by all contributors of a PR label Dec 20, 2023
@oxzi
Copy link
Member Author

oxzi commented Dec 20, 2023

Marked as draft, as a fork of icinga-testing is used - Icinga/icinga-testing#25 - and there might be additional tests to be written.

@oxzi oxzi force-pushed the init-integration-tests branch 7 times, most recently from 440e6a4 to aa903b8 Compare January 4, 2024 08:46
@oxzi
Copy link
Member Author

oxzi commented Jan 4, 2024

By switching to the already existing Dockerfile, some permission difficulties have risen when host_uid != container_uid && container_uid != 0. To not always have to wait for the CI to time out, make sure that the UID within the container does not matches your host user's UID, e.g., by editing the Dockerfile.

Fixed by temporary setting an open umask, as mkfifo's mode is NANDed with the current mask, and chmoding the configuration file.

@oxzi oxzi force-pushed the init-integration-tests branch from aa903b8 to 16b1839 Compare January 4, 2024 09:32
@julianbrost
Copy link
Collaborator

Fixed by temporary setting an open umask, as mkfifo's mode is NANDed with the current mask, and chmoding the configuration file.

Why not just use chmod in both cases? If it just adds permissions, that's perfectly fine and nicer since the umask is process state.

I have two other ideas that might also help here, but are probably out of scope here, but would be useful independent of testing:

  1. Allow configuring the container from environment variable, i.e. remove the need for mounting things as a volume. (Note: in contrast to what the Icinga DB container image does, I wouldn't generate a config file from the environment in a separate entrypoint but rather try to do this in the daemon directly.)
  2. Implement a webhook notification channel, in general, that would be something quite useful (but to be really useful, this would need a way to transform the requests to actually be useful with other services, but having one without this feature would be a starting point).

By the way, it's not NAND, as this negates the output of AND, but to apply the umask, the umask input is negated (i.e. masked away).

@oxzi oxzi force-pushed the init-integration-tests branch 2 times, most recently from 04cbbc5 to 2e770f1 Compare January 4, 2024 15:37
@oxzi
Copy link
Member Author

oxzi commented Jan 4, 2024

Fixed by temporary setting an open umask, as mkfifo's mode is NANDed with the current mask, and chmoding the configuration file.

Why not just use chmod in both cases? If it just adds permissions, that's perfectly fine and nicer since the umask is process state.

Sounds fair. Now both cases are using chmod.

I have two other ideas that might also help here, but are probably out of scope here, but would be useful independent of testing:

1. Allow configuring the container from environment variable, i.e. remove the need for mounting things as a volume. (Note: in contrast to what the Icinga DB container image does, I wouldn't generate a config file from the environment in a separate entrypoint but rather try to do this in the daemon directly.)

In theory, this is already possible: https://github.com/Icinga/icinga-testing/blob/1ef6e79ddf1ffaaae423a3f861774a24365f25e0/internal/services/notifications/docker.go#L91

I can change the logic to COPY the new config into a new derived container image. However, the current FIFO approach would then still need a volume mount unless being changed, as suggested in the second bullet point.

2. Implement a webhook notification channel, in general, that would be something quite useful (but to be really useful, this would need a way to transform the requests to actually be useful with other services, but having one without this feature would be a starting point).

I used the file (or file-like) approach as it was the simplest thing to implement I could think of. Furthermore, I had this as a real use case in mind, as, for SLA and other compliance thingies, one might want to keep a "hard copy" of all event logs lying around.

However, this can also be changed to a more or less generic web hook, i.e., being configurable through Go's template engine. By doing so one can get rid of the other volume mount.

By the way, it's not NAND, as this negates the output of AND, but to apply the umask, the umask input is negated (i.e. masked away).

Ehm.. yes ._.

@julianbrost
Copy link
Collaborator

I have two other ideas that might also help here, but are probably out of scope here, but would be useful independent of testing:

  1. Allow configuring the container from environment variable, i.e. remove the need for mounting things as a volume. (Note: in contrast to what the Icinga DB container image does, I wouldn't generate a config file from the environment in a separate entrypoint but rather try to do this in the daemon directly.)

In theory, this is already possible: Icinga/icinga-testing@1ef6e79/internal/services/notifications/docker.go#L91

Oh, that's not what I had in mind. I mean that you can start the container with variables similar to https://github.com/icinga/docker-icingadb/#usage. That's something we'll eventually want for the container image anyways.

@oxzi
Copy link
Member Author

oxzi commented Jan 4, 2024

I have two other ideas that might also help here, but are probably out of scope here, but would be useful independent of testing:

  1. Allow configuring the container from environment variable, i.e. remove the need for mounting things as a volume. (Note: in contrast to what the Icinga DB container image does, I wouldn't generate a config file from the environment in a separate entrypoint but rather try to do this in the daemon directly.)

In theory, this is already possible: Icinga/icinga-testing@1ef6e79/internal/services/notifications/docker.go#L91

Oh, that's not what I had in mind. I mean that you can start the container with variables similar to https://github.com/icinga/docker-icingadb/#usage. That's something we'll eventually want for the container image anyways.

Then let's use the third way to configure dockerized software through environment variables - also fine for me. I would give this one a shot as this seems like a good small change to depend this PR on.

@julianbrost
Copy link
Collaborator

Then let's use the third way to configure dockerized software through environment variables

What are the other two ways? Currently, the only option is to mount a config file into the container, but for me, that's more of a temporary workaround as there is nothing implemented to configure it via the environment, but once there is, that's the preferred way for me. What's the other remaining way you have in mind?

@oxzi
Copy link
Member Author

oxzi commented Jan 5, 2024

Then let's use the third way to configure dockerized software through environment variables

What are the other two ways? Currently, the only option is to mount a config file into the container, but for me, that's more of a temporary workaround as there is nothing implemented to configure it via the environment, but once there is, that's the preferred way for me. What's the other remaining way you have in mind?

The other two ways were mounting the configuration within the container (as the current code does) or deriving a new Docker image with an updated config (as I proposed earlier in #144 (comment))

Another topic: regarding a web request channel instead of a file channel. When establishing a connection from the dockerized icinga-notifications daemon to a web server, this web server also needs to be within the Docker network (or the icinga-notifications daemon must be bound to the host network). This breaks the current setup of having the test running on the host and the daemons within Docker. Of course, we could speak HTTP over an Unix domain socket, but this would require a mount again.

@julianbrost
Copy link
Collaborator

Another topic: regarding a web request channel instead of a file channel. When establishing a connection from the dockerized icinga-notifications daemon to a web server, this web server also needs to be within the Docker network (or the icinga-notifications daemon must be bound to the host network). This breaks the current setup of having the test running on the host and the daemons within Docker. Of course, we could speak HTTP over an Unix domain socket, but this would require a mount again.

Yes, that's part of why I consider this out of scope for now. Some notes on this:

  • It's possible to access services running on the host from a container: https://docs.docker.com/desktop/networking/#i-want-to-connect-from-a-container-to-a-service-on-the-host (somehow that doesn't work out of the box for me as advertised but it does when starting the container with --add-host host.docker.internal:host-gateway, so technically possible).
  • Another idea that exists would be to run the whole test binary (i.e. the one built by go test) inside Docker, giving it direct access to the network. However, this means that this container needs access to the Docker socket. (Main motivation for that would be that this could make it easier to run the tests on non-Linux hosts where it's not necessarily possible to simply add services using the Docker-internal IP addresses.)

@oxzi oxzi force-pushed the init-integration-tests branch from 2e770f1 to ee9b049 Compare January 9, 2024 17:09
@oxzi oxzi force-pushed the init-integration-tests branch from ee9b049 to 479a43c Compare January 12, 2024 09:38
This was referenced Jan 12, 2024
@oxzi oxzi force-pushed the init-integration-tests branch from 479a43c to ddf970a Compare January 18, 2024 12:53
@oxzi
Copy link
Member Author

oxzi commented Jan 18, 2024

I have made some structural changes to this PR and its linked PR Icinga/icinga-testing#27. As just written over there, this PR is now based on the environment variable configuration (#148) and the webhook channel (#149).

A local web server to receive webhook messages will be launched on the host and is being bound to the host's address in the Docker Network, which is the Gateway address in Docker's lingo. After playing around with some approaches, this seems to be the easiest one to work out of the box without having to reconfigure Docker, as it is necessary, e.g., for using host.docker.internal on a Linux-based OS.

@oxzi oxzi force-pushed the init-integration-tests branch from ddf970a to 2ea4d30 Compare January 18, 2024 13:04
@oxzi
Copy link
Member Author

oxzi commented Jan 18, 2024

FTR, I have also pushed my testing branch x-integration-tests, including the other two necessary PR branches, created through git-assembler. Feel free to use this one to check how everything plays together.

Heavily inspired by the already existing integration tests from Icinga
DB, the icinga-testing project was extended to support Icinga
Notifications[0].

Based on those changes, an initial integration test was implemented for
Icinga Notifications, launching and configuring PostgreSQL, Icinga 2,
and Icinga Notifications. Afterwards, a webhook channel is used to catch
notifications.

[0] Icinga/icinga-testing#27
@oxzi oxzi force-pushed the init-integration-tests branch from 2ea4d30 to 4992b13 Compare January 18, 2024 13:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla/signed CLA is signed by all contributors of a PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants