Changes to work with Pod Security Restricted and OpenShift

[wkbrd]

This includes changes to add support for running within Pod Security Admission in "Restricted" profile (the most restricted policy).
Details:

• https://kubernetes.io/docs/concepts/security/pod-security-standards/
• https://kubernetes.io/docs/concepts/security/pod-security-admission/

It also includes OpenShift support for running with a flexible UID (removing runAsUser, runAsGroup, and fsGroup on OpenShift, which are managed by OpenShift for security reasons).

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[sklarsa]

This is looking good, just looking into a tangentially-related issue.

Since our linux Dockerfile does not have a USER directive, combined with the fact that we're mounting /var/lib/questdb/db, .checkpoint, and snapshot by subpaths, we get mixed-ownership in the db root when we add the security context (run as user 10001):

drwxrwxrwx 2 root    root    4.0K Oct  6 18:04 .checkpoint
drwxr-xr-x 2 questdb questdb 4.0K Oct  6 18:04 conf
drwxrwxrwx 9 root    root    4.0K Oct  6 18:04 db
-rw-rw-r-- 1 questdb questdb  503 Oct  6 18:04 hello.txt
drwxr-xr-x 2 questdb questdb 4.0K Oct  6 18:04 import
drwxr-xr-x 3 questdb questdb 4.0K Oct  6 18:04 public
drwxrwxrwx 2 root    root    4.0K Oct  6 18:04 snapshot

I was reading about this, and while it's ok in this case because the dirs have 777 permissions, this might not be guaranteed across all k8s distributions and volume drivers.

Fixing this is relatively straightforward. We should remove the following subpath mounts (in statefulset.yaml)

          - name: {{ include "questdb.fullname" . }}
            mountPath: {{ .Values.questdb.dataDir }}/db
            subPath: db/
          - name: {{ include "questdb.fullname" . }}
            mountPath: {{ .Values.questdb.dataDir }}/.checkpoint
            subPath: .checkpoint/
          - name: {{ include "questdb.fullname" . }}
            mountPath: {{ .Values.questdb.dataDir }}/snapshot
            subPath: snapshot/

and replace them with

          - name: {{ include "questdb.fullname" . }}
            mountPath: {{ .Values.questdb.dataDir }}

I've only tested this in a kind cluster, but I believe that this should be safe to change... we're mounting the entire directory instead of just the subpaths. I still need to confirm that the config subpaths will mount correctly will mount correctly (assuming that we're now mounting their parent /var/lib/questdb dir). That's probably why I used subpaths in the first place...

[wkbrd]

FYI, I am working with the 9.0.3-rhel image at the moment.

The container does appear to set 'USER questdb'. For compliance with OpenShift, the container should be authored with 'USER 10001'. I just created questdb/questdb#6238 which should address this within the questdb Dockerfile (though I do not have a mechanism to test/confirm this at this time).

Are you sure that when running the container it is running as root? I would expect it would be running as questdb (10001) and therefore the files would be written OK with the Helm chart as authored. It appears to be working in the places where I have been testing.

[sklarsa]

FYI, I am working with the 9.0.3-rhel image at the moment.

Perfect, that's what I was going to recommend. We have a multistage build with 2 separate outputs (one for rhel and one without) in our main repo. I was testing using the non-rhel (default) image (which is what the majority of our users are running), which is why we were getting different results.

I'm a bit hesitant to make a change to our Dockerfile without further testing to ensure that we don't break anyone's environments on upgrade. For now, to maintain compatibility, and also because it's cleaner, let's remove the subpath mounts as mentioned in my above comment. I'll keep Dockerfile discussion in the other issue. Thanks!

Edit:

After reviewing the PR, I've decided to keep the Dockerfile as-is to maintain wide compatibility. We use gosu to step down into a non-root user before running the database. I think I was "root" in my previous example because I was inside a shell that was a second process inside the container, which was not impacted by the gosu logic in the entrypoint. But I'm still looking into this.

[wkbrd]

Removing the paths will not work in conjunction with overriding the config files. I think the subpaths need to stay. Is there a reason we need to change that? I'd prefer to keep that separate from this PR.

[sklarsa]

Removing the paths will not work in conjunction with overriding the config files

That's what I initially thought as well, but after a bit of testing it appears that subPath mounts of individual files (say, from a ConfigMap key) will work inside a mounted volume.

Take a look at this values.yaml

questdb:
  serverConfig:
    enabled: true
    options:
      shared.worker.count: "3"

and the resulting file inside the container

questdb@questdb-0:~$ cat conf/server.conf 
metrics.enabled = true
shared.worker.count = 3

The subpath volumeMounts correctly mount the configmap values to their respective files!

Is there a reason we need to change that? I'd prefer to keep that separate from this PR.

I'm trying to avoid potential permissions issues based on changes to the helm chart. If we remove the snapshot/.checkpoint/db subpaths, we get a nice clean questdb user ownership across all db subdirectories:

questdb@questdb-0:~$ ls -lah
total 32K
drwxrwxrwx 6 root    root    4.0K Oct  7 17:42 .
drwxr-xr-x 1 root    root    4.0K Oct  3 13:59 ..
-rw------- 1 questdb questdb   16 Oct  7 17:42 .bash_history
drwxr-xr-x 2 questdb questdb 4.0K Oct  7 17:40 conf
drwxr-xr-x 9 questdb questdb 4.0K Oct  7 17:42 db
-rw-rw-r-- 1 questdb questdb  503 Oct  7 17:42 hello.txt
drwxr-xr-x 2 questdb questdb 4.0K Oct  7 17:40 import
drwxr-xr-x 3 questdb questdb 4.0K Oct  7 17:40 public

and the group for server.conf is correct (with its own subpath)

questdb@questdb-0:~$ ls -lah conf
total 68K
drwxr-xr-x 2 questdb questdb 4.0K Oct  7 17:40 .
drwxrwxrwx 6 root    root    4.0K Oct  7 17:42 ..
-rw-r--r-- 1 questdb questdb 1.4K Oct  7 17:40 log.conf
-rw-r--r-- 1 questdb questdb  52K Oct  7 17:40 mime.types
-rw-r--r-- 1 root    questdb   47 Oct  7 17:42 server.conf

Compare this to my previous example:

drwxrwxrwx 2 root    root    4.0K Oct  6 18:04 .checkpoint
drwxr-xr-x 2 questdb questdb 4.0K Oct  6 18:04 conf
drwxrwxrwx 9 root    root    4.0K Oct  6 18:04 db
-rw-rw-r-- 1 questdb questdb  503 Oct  6 18:04 hello.txt
drwxr-xr-x 2 questdb questdb 4.0K Oct  6 18:04 import
drwxr-xr-x 3 questdb questdb 4.0K Oct  6 18:04 public
drwxrwxrwx 2 root    root    4.0K Oct  6 18:04 snapshot

Even though .checkpoint, db, and snapshot are 777, I'm not sure where that is being set, even after a bit of research. It may be CSI driver-specific, which may introduce breaking changes for users depending on which driver they're using. I'd prefer to use the above method (subpaths for config files and 1 volume mount for the entire data dir) to guarantee that the container user is the owner of the files.

Can you confirm this behavior on Openshift?

[wkbrd]

@sklarsa : I can confirm that the change you requested is working. However, for OpenShift to fully work, I am testing with a Helm chart that has further changes. This PR does not address temporary locations that need to be mounted as emptydir's since the running UID within OpenShift is not able to write into those places (it is a random UID). I plan to recommend that in a later PR.

With these changes incorporated, do you have any further feedback for this PR?

[sklarsa]

@wkbrd lgtm thanks!

Let's open up a separate PR for the temp dir handling. Perhaps we can roll that into the openshift flag you added in this PR?

Also can you please sign the CLA so we can merge these? Thanks!