openshift: improve compatibility #131
Conversation
- update container image permissions and user handling - add security context related docs to Helm chart values - update helm chart deployment docs
|
@blazdivjak Sorry for the delay, but thanks for the contribution. To be honest we don't know a lot about Openshift, maybe others from the community could step in to review this. |
|
I like the changes you did. I did not use helm or the deployments but your changes to the Dockerfiles work perfectly and deploy on Openshift without root or any other extra-permissions. |
| @@ -54,9 +54,21 @@ EXPOSE 1883 8883 8080 44053 4369 8888 \ | |||
| 9100 9101 9102 9103 9104 9105 9106 9107 9108 9109 | |||
|
|
|||
|
|
|||
| VOLUME ["/vernemq/log", "/vernemq/data", "/vernemq/etc"] | |||
There was a problem hiding this comment.
Can you explain why you've removed the /vernemq/etc volume?
There was a problem hiding this comment.
- Docker container runtime modifies permissions for folders listed as volumes when spinning up a container if no special mount point is defined.
- Helm chart does not mount anything to
/vernemq/etc. - This creates issues when running as random user.
There was a problem hiding this comment.
With your addition would mounting a custom /vernemq/etc/vernemq.conf.local still work?
There was a problem hiding this comment.
@dergraf it shouldn't make a difference, and @blazdivjak is probably right, it shouldn't have been there in the first place most likely. You can still mount that file from within docker run or k8s even without the volume statement.
There was a problem hiding this comment.
Ok, thanks. Does this mean that we can get rid of log and data too?
|
Sorry for being late to the party. However, the changes are sound to me, but I wonder whether this has been tested outside OpenShift given the various permission changes (apparently shouldn't be an issue, but better safe than sorry). So thumbs up, especially if somebody tested this on standard k8s :) |
|
I did test it on k8s as well. Not extensively however. |
|
@greggiles thanks a lot for your testing and vote to merge this. |
|
@ioolkos this looks good to me. Maybe a small comment, instead of adding the new file |
This pull request includes updates for Openshift compatibility.
Tested:
Includes:
Container images: https://hub.docker.com/r/blazdivjak/docker-vernemq