Share the PostgreSQL socket with the sidecars containers

[frittentheke]

While PR https://github.com/zalando/postgres-operator/pull/736 already enabled any additional volumes, including emptyDirs, this is intended to be per-cluster, so it needs to be specified in the postgresql spec of every cluster.

This PR creates an emptyDir for /var/run to be shared between the postgresql container and the (potential) sidecars. This allows tools like the postgres-exporter (https://github.com/wrouesnel/postgres_exporter) to be used without any configuration or handling of credentials.

Certainly another option would be to port the additionalVolumes functionality to also accept some global configuration via the operator config. But it seems that there are not that many cases for volume configurations that can be the same for every cluster, so I figured a switch to just enable the socket sharing could be an helpful addition.

[lebenitza]

Looking forward to this. Found that the hard way I cannot use unix socket with sidecar postgres_exporter now :(

[frittentheke]

@lebenitza I did a rebase to the current master to have the merge cleanly. But it's up to @FxKu to accept this PR and pick it for the 1.6 release.

[frittentheke]

@FxKu @CyberDem0n are you still interested in this change? Do you need me to make any changes or would a rebase be enough?

[FxKu]

tbh: I have not really a clue what this is about. Seems like documentation for this parameter is missing. Maybe you can add it + rebasing and we can merge.

[frittentheke]

tbh: I have not really a clue what this is about. Seems like documentation for this parameter is missing. Maybe you can add it + rebasing and we can merge.

@FxKu I rebased the change and added the missing piece of documentation on this config flag. Currently the e2e tests fail somewhere around the connection limit tests. This seems rather unrelated to what I added. Could you kindly PTAL.

[FxKu]

1. Can you rename the option to enable_sidecars_pg_socket maybe? Than we have the option in alphabetical order in the CRD files. I wonder if the name pg_socket is appropriate because it does not appear in the code. You're simply adding the run volume of Postgres.
2. We were thinking if really all sidecars should get access to this volume? Or if there should be an extra allow list instead?
3. Do sidecars get access to Postrges with trust connection with this feature?

[frittentheke]

Excuse my late reply @FxKu

1. Can you rename the option to enable_sidecars_pg_socket maybe? Than we have the option in alphabetical order in the CRD files. I wonder if the name pg_socket is appropriate because it does not appear in the code. You're simply adding the run volume of Postgres.

I certainly could rename it. I named it pg_socket because that is the only thing living at this location.

2. We were thinking if really all sidecars should get access to this volume? Or if there should be an extra allow list instead?

That's a valid point. I naively assumed at sidecars of a postgresql database pod would likely be talking psql to it. But could very much but an outbound proxy or else which does not need the socket at all.

3. Do sidecars get access to Postrges with `trust` connection with this feature?

I believe yes. But this comes down to the pg_hba.conf (https://www.postgresql.org/docs/14/auth-pg-hba-conf.html) which is provided by Spilo (https://github.com/zalando/spilo/blob/c41394b236f604938e947a7eefb8441681331c32/postgres-appliance/scripts/configure_spilo.py#L313).

But mind you trust does not mean "only superuser" within the database (https://www.postgresql.org/docs/14/auth-trust.html). But still, maybe switching this to password/md5 (https://www.postgresql.org/docs/14/auth-password.html) for local might make sense.

Looking at all three questions and thinking my idea through again I believe there is a MUCH simpler approach:

1. Use an EmptyDir volume for /var/run/postgresql of the main container - by default, no option there
2. Leave it to each individual sidecar spec to simply mount this volume.

No options, no exclude list, no exposure of the socket to all sidecars. Just a tiny bit of documentation to the sidecar section (https://github.com/zalando/postgres-operator/blob/master/docs/user.md#sidecar-support) explaining how to configure the VolumeMount.

Let me know if I totally wrong or if you like the idea. If so, I gladly push an update to the PR just changing the PodSpec in regards to the postgresql container and be done with it.

[tpo]

@frittentheke (what a funny handle ;-) wrote:

Looking at all three questions and thinking my idea through again I believe there is a MUCH simpler approach:

1. Use an EmptyDir volume for `/var/run/postgresql`  of the main container - by default, no option there

2. Leave it to each individual sidecar spec to simply mount this volume.

No options, no exclude list, no exposure of the socket to all sidecars. Just a tiny bit of documentation to the sidecar section (https://github.com/zalando/postgres-operator/blob/master/docs/user.md#sidecar-support) explaining how to configure the VolumeMount.

Let me know if I totally wrong or if you like the idea. If so, I gladly push an update to the PR just changing the PodSpec in regards to the postgresql container and be done with it.

I have verified today that this approach indeed does work:

apiVersion: acid.zalan.do/v1
kind: postgresql
metadata:
  name: qgc-cluster
  namespace: qgiscloud
spec:
  additionalVolumes:
  - mountPath: /var/run/postgresql/
    name: var-run-postgres
    targetContainers:
    - all
    volumeSource:
      hostPath:
        # the directory will be created for you on the host, however make sure 
        # it's at some place that makes sense to you
        path: /srv/k8s/var/run/postgresql/
  sidecars:
  # some random sidecar for demo purposes  
  - image: k8s.gcr.io/echoserver
    name: echoserver
  [...etc...]

I was able access the DB from the sidecar via the socket in /var/run/postgresql/.

I propose to document this under https://postgres-operator.readthedocs.io/en/refactoring-sidecars/user/#sidecar-support . OK @FxKu ?

[frittentheke]

I propose to document this under https://postgres-operator.readthedocs.io/en/refactoring-sidecars/user/#sidecar-support . OK @FxKu ?

@tpo just added the missing documentation on accessing the socket from sidecars. @FxKu @Jan-M @CyberDem0n could you kindly let me know if this PR works for you or if I should make any changes?

[FxKu]

@frittentheke this looks nice to me now. Thanks a lot for pushing and sorry again for the huge delay. I would like to merge. Can we have one unit test in k8sres_test.go where we check that the volume mounts are created when the option is enabled and that it's not when disabled? Ideally with a mocked cluster, but if you lack time than something like TestShmVolume should at least be there.

[FxKu]

:+1:

[frittentheke]

@FxKu if you are happy, could you kindly tell me if this can be merged as is or if you want any changes?

[jopadi]

👍

[FxKu]

@frittentheke in August I wished for a unit test that I did not get from you :smiley: So I decided to merge it now and provide the test myself.

[frittentheke]

@frittentheke in August I wished for a unit test that I did not get from you smiley So I decided to merge it now and provide the test myself.

Uhh sorry, I did not even scroll up to your previous comments when I noticed some movement in the PR. Thanks for doing the test yourself then and Guten Rutsch.