Make it easier to run in Kubernetes #203
Conversation
- Let the php container run the inet supervisord for the bg workers still - Properly configure the cron container to exec cron - Add configuration to optionally change the sock file location for php-fpm, allows us to specify a shared file between containers in a pod - make new entrypoint files executable - Set the php config value for `session.cookie_domain` so that it doesn't use the default of ''. When empty it falls back to the hostname which will be different per pod, meaning that each pod will handle session requests separately, which breaks things like OIDC.
|
Thank you for this PR @jhuntwork , this is awesome stuff 🎄 I will need a bit of time due to day-job constraints, but will get to it eventually. Feel free to reach out to me on Gitter, so we can iterate quickly on this. |
|
@oivindoh can you give it a look? Looks reasonable to be, but I would like this not to clash with your current way of doing things on K8S. Likewise, if you have suggestions, please add them here. |
|
Sorry, it seems I never actually hit send on the comment I wrote here earlier... I think this looks good and is a sensible change. Perhaps we could default to standard behaviour if the container name is not php,nginx or cron - otherwise we'll need to surface some kind of breaking change note to end-users. I'll give this a whirl in my test env before the end of the week. |
| # start supervisord using the main configuration file so we have a socket interface | ||
| /usr/bin/supervisord -c /etc/supervisor/supervisord.conf | ||
| if [ -n "$KUBERNETES_SERVICE_HOST" ]; then | ||
| case "$CONTAINER_NAME" in |
There was a problem hiding this comment.
Really a shame the container name isn't exposed to the container environment by default.
| @@ -30,6 +30,11 @@ if [[ ! -p /tmp/cronlog ]]; then | |||
| mkfifo -m 777 /tmp/cronlog | |||
| fi | |||
|
|
|||
| if [ -n "$KUBERNETES_SERVICE_HOST" ]; then | |||
| tail -f /tmp/cronlog & | |||
| exec cron -l -f | |||
There was a problem hiding this comment.
We're missing some setup for the default cron jobs to be able to run here (www-data /var/www/MISP/app/Console/cake Admin updateGalaxies and friends). Not sure it's feasible to run these in their own container.
Maybe the best solution is to rewrite them all to run API calls / pymisp against localhost with supplied admin_key?
There was a problem hiding this comment.
On second thought, a better solution would befor us to supply an example set of CronJob manifests to deploy alongside MISP.
| @@ -28,6 +28,7 @@ change_php_vars() { | |||
| sed -i "s|.*session.save_path = .*|session.save_path = '$(echo $REDIS_HOST | grep -E '^\w+://' || echo tcp://$REDIS_HOST):$REDIS_PORT?auth=${ESCAPED}'|" "$FILE" | |||
| sed -i "s/session.sid_length = .*/session.sid_length = 64/" "$FILE" | |||
| sed -i "s/session.use_strict_mode = .*/session.use_strict_mode = 1/" "$FILE" | |||
| sed -i "s|session.cookie_domain = .*|session.cookie_domain = ${BASE_URL}|" "$FILE" | |||
|
This seems to work well (with the exception of default cron jobs failing to run), with the three containers set up and the CONTAINER_NAME var supplied to all, as well as a shared emptyDir volume for /var/run/php between nginx and php containers. I added a readinessprobe to count the number of socket files present there for both the nginx and php containers in my test deployment. readinessProbe:
exec:
command:
- /bin/sh
- -c
- 'test $(ls /run/php/php*-fpm*sock | wc -l) -eq 1'Would be really nice to get all the PHP logs redirected to stdout. There's CakeLog in bootstrap.php responsible for app/tmp/logs/[error|debug].log, then here are also other hardcoded log files like ProcessTool (exec-errors.log), ServerSyncTool (server-sync.log), AppExceptionRenderer (fatal_error.log). The hardcoded ones certainly require code changes in MISP upstream. I expected to be able to at least change boostrap.php to write to php://stdout, but so far no luck on that front here. |
| @@ -68,5 +68,22 @@ export PHP_SESSION_COOKIE_SAMESITE=${PHP_SESSION_COOKIE_SAMESITE:-Lax} | |||
| export NGINX_X_FORWARDED_FOR=${NGINX_X_FORWARDED_FOR:-false} | |||
| export NGINX_SET_REAL_IP_FROM=${NGINX_SET_REAL_IP_FROM} | |||
|
|
|||
| # start supervisord using the main configuration file so we have a socket interface | |||
| /usr/bin/supervisord -c /etc/supervisor/supervisord.conf | |||
| if [ -n "$KUBERNETES_SERVICE_HOST" ]; then | |||
There was a problem hiding this comment.
I'd change this to if [ -n "$KUBERNETES_SERVICE_HOST" ] && [ -n "$CONTAINER_NAME" ]; then to ensure we just do the default thing if CONTAINER_NAME is missing. This ensures existing setups aren't immediately broken by updating.
These changes make running in Kubernetes simpler. The main goal here is to split out nginx, php-fpm and crond as separate processes that execute as PID 1 in their own container within a pod. This lets Kubernetes itself supervise those processes instead of the intermediary supervisord. For example, it allows Kubernetes to restart nginx or php-fpm individually in a container if that becomes necessary.
We tried to make these changes as unobtrusive as possible, so that it will be easier to test and doesn't interrupt existing functionality. We did this by testing for the presence of the
KUBERNETES_SERVICE_HOSTvariable which will be present if this container is running in that environment. We then added new k8s specific entrypoint files that source and generally follow the logic of the existing entrypoint files, with the exception two major rules:This is also designed to use the same image for all three containers, but just modify their entrypoint script slightly for each case.
There are still a few outstanding things that should probably be improved:
Looking forward to feedback and happy to adjust where ever it makes sense.