adopt netxcloud.containerport in FPM deployment, allow rootless nginx - nd2 try #444
Conversation
is netxcloud.containerport is set, it will also configure the port for nginx. This allows nginx running in a rootless container e.g. by using their official `nginxinc/nginx-unprivileged` image. pid is written to /tmp for that reason. Some enhanced configs parameter for service. Signed-off-by: poggenpower <[email protected]>
Signed-off-by: poggenpower <[email protected]>
Signed-off-by: poggenpower <[email protected]>
| worker_processes auto; | ||
|
|
||
| error_log /var/log/nginx/error.log warn; | ||
| pid /var/run/nginx.pid; | ||
| pid /tmp/nginx.pid; |
There was a problem hiding this comment.
will this break any normal flows not using a non-root container?
There was a problem hiding this comment.
This pid stuff is not really required in containers, as the container get removed if something goes wrong, to pid to check and kill afterwards etc. As root has also permission to /tmp this change doesn't harm.
|
thanks for all you hard work, @poggenpower ! |
| @@ -44,7 +32,7 @@ data: | |||
| } | |||
|
|
|||
| server { | |||
| listen 80; | |||
| listen {{ .Values.nginx.containerPort | default "80" }}; | |||
There was a problem hiding this comment.
Sorry to miss this before it merged, but I'm curious why not just .Values.nextcloud.containerPort? I'm not seeing a reason why a new variable was necessary, and now it seems like it's necessary to set both variables. The deployment.yaml template still uses .Values.nextcloud.containerPort for the containerPort of the nginx container.
There was a problem hiding this comment.
This was the case in my original PR, but we discussed this here #440 (comment) and ended up with an extra var.
There was a problem hiding this comment.
we could change the default to be containerPort, so the new line would be like:
listen {{ .Values.nginx.containerPort | default .Values.nextcloud.containerPort }};
Would that help?
There was a problem hiding this comment.
Thanks for pointing out the previous discussion. I don't follow the argument for granularity here, but if that feels right to you both then we should keep it.
The template update to fall back to the primary container port variable seems convenient, but I'm fine to leave things as they are.
It's only because I didn't understand any current or future scenarios where these variables would ever have different values, and didn't see the previous discussion, that I suggested just removing the nginx one.
replaces #440
as the old PR got stuck in git fanciness, simply resubmit as a new PR.
Seems to be the easiest way