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

Add healthchecks to docker-compose.yml #36

Merged
merged 4 commits into from
Apr 24, 2024
Merged

Conversation

ftoppi
Copy link
Contributor

@ftoppi ftoppi commented Mar 29, 2024

Hello,

here's a small PR to replace redis with keydb due to redis license changes and to add healthchecks to services.

Due to Redis licensing not being opensource anymore, here's a drop-in replacement with an opensource license.

Source:
https://redis.com/blog/redis-adopts-dual-source-available-licensing/
https://github.com/Snapchat/KeyDB/blob/main/COPYING
Added healthchecks for redis, db and misp-core services.
misp-core and misp-modules now wait for redis and db to be healthy before starting.
@@ -12,7 +12,7 @@ services:
- "SMARTHOST_ALIASES=${SMARTHOST_ALIASES}"

redis:
image: redis:7.2
image: eqalpha/keydb:x86_64_v6.3.4
Copy link
Collaborator

Choose a reason for hiding this comment

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

This image is x64 only while the Redis image is ARM as well. Would it be possible to remove the arch modifier here?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Note that unless MISP changes the default key-value storage, we should only document this alternative in the README.md rather than modify the docker-compose.yml file

Copy link
Contributor Author

@ftoppi ftoppi Mar 31, 2024

Choose a reason for hiding this comment

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

Fine by me. All their images are arch-specific except eqalpha/keydb:latest which is both for arm64 and x86_64.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We should also reference https://github.com/valkey-io/valkey if a docker image is available.

db:
condition: service_healthy
healthcheck:
test: curl -ks https://localhost/users/login > /dev/null || exit 1
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this check run inside the container? In other words, Is localhost the correct setting regardless of our BASE_URL?

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, it's run inside the container.
You can see below I don't expose ports in my stack and it's still healthy:

$ docker compose ps 2> /dev/null
NAME                  IMAGE                                          COMMAND                  SERVICE        CREATED        STATUS                  PORTS
misp-db-1             mariadb:10.11                                  "docker-entrypoint.s…"   db             43 hours ago   Up 43 hours (healthy)   3306/tcp
misp-mail-1           ixdotai/smtp                                   "/bin/entrypoint.sh …"   mail           43 hours ago   Up 43 hours             25/tcp
misp-misp-core-1      ghcr.io/misp/misp-docker/misp-core:latest      "/entrypoint.sh"         misp-core      42 hours ago   Up 42 hours (healthy)
misp-misp-modules-1   ghcr.io/misp/misp-docker/misp-modules:latest   "/usr/local/bin/misp…"   misp-modules   43 hours ago   Up 43 hours
misp-redis-1          eqalpha/keydb:x86_64_v6.3.4                    "docker-entrypoint.s…"   redis          43 hours ago   Up 43 hours (healthy)   6379/tcp

Copy link
Collaborator

@ostefano ostefano 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 good stuff, but I think it's too early to replace redis.

Can we add the healthchecks first, and only when better docker images are published, move to a new key value store?

Alternatively, it should be easy to just document the alternatives available in the README.

image: redis:7.2
image: eqalpha/keydb:x86_64_v6.3.4
healthcheck:
test: keydb-cli ping || exit 1
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is the alternative command if were to use Redis here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

redis-cli ping

@ostefano ostefano added documentation Improvements or additions to documentation enhancement New feature or request labels Apr 5, 2024
Copy link
Collaborator

@ostefano ostefano left a comment

Choose a reason for hiding this comment

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

Will merge if changes to the underlying image are removed.

Also, note that we decided to migrate to valkey, so healthcheck commands might need updating.

@ftoppi
Copy link
Contributor Author

ftoppi commented Apr 21, 2024

Will merge if changes to the underlying image are removed.

Also, note that we decided to migrate to valkey, so healthcheck commands might need updating.

I figured everyone would move to valkey but it wasn't available at the time.

This is better?

@ostefano ostefano changed the title docker-compose.yml: replace redis + add healthchecks Add healthchecks to docker-compose.yml Apr 21, 2024
@ostefano
Copy link
Collaborator

Looks good.

Can you rebase and squash? Otherwise I will do it when merging (will wait for next MISP release).

@ostefano ostefano merged commit a20eece into MISP:master Apr 24, 2024
1 check failed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants