Validate and use pgbouncer logfile config #4252
Conversation
internal/collector/pgbouncer.go
Outdated
| create_directory := false | ||
| if directory != "/tmp" { | ||
| create_directory = true | ||
| } |
There was a problem hiding this comment.
I ran into an error when I had set the logfile conf to a non tmp location -- our collector startup has
` + shell.MakeDirectories(logDir, path.Join(logDir, "receiver"))
which would seem to have solved the issue, but I suppose pgbouncer might start up and error out before this. And the pgbouncer container should manage its own logfile location.
There was a problem hiding this comment.
Yeah. I used the postgres-startup container for this. This is one of the situations I would love to solve with a Go sidecar.
There was a problem hiding this comment.
🤔 In this moment, we could augment the entrypoint.
Command: []string{"sh", "-c", "--",
mkdirs + `; exec "$@"`, "--",
"pgbouncer", iniFileAbsolutePath},There was a problem hiding this comment.
I just re-tested in two scenarios (with OTEL, without OTEL) and even without any special setup of the files, it seemed to work fine. I wonder if the error I saw before was related to the fsGroup not being set.
(Also, when I run shell.MakeDirectories, now I get an error about chmod not being allowed.)
There was a problem hiding this comment.
chmod not being allowed
What storage do you have? That sounds like NFS, maybe.
There was a problem hiding this comment.
I've been using GKE for testing this, I want to run some more tests to double-check everything
There was a problem hiding this comment.
ok, adding back some dir management to the entrypoint, seems fine -- as long as the location exists
|
|
||
| // PodSecurityContext returns a v1.PodSecurityContext for cluster that can write | ||
| // to PersistentVolumes. | ||
| func PodSecurityContext(fsgroup int64, supplementalGroups []int64, openshift bool) *corev1.PodSecurityContext { |
There was a problem hiding this comment.
🤔 I think I'd prefer the caller to pass in fsgroup = 0 when they know its OpenShift, but a single place with this knowledge is good, too.
🤔 🤔 PgBouncer is deployed from a PostgresCluster spec which has an isOpenShift override.
There was a problem hiding this comment.
on PodSecurityContext, fsgroup is a pointer (with omitempty) so we don't want to set it at all, right?
| @@ -26,7 +26,12 @@ type PGBouncerConfiguration struct { | |||
|
|
|||
| // Settings that apply to the entire PgBouncer process. | |||
| // More info: https://www.pgbouncer.org/config.html | |||
| // | |||
| // # Logging | |||
| // +kubebuilder:validation:XValidation:rule=`!has(self.logfile) || self.logfile.endsWith('.log')`,message=`logfile config must end with '.log'` | |||
There was a problem hiding this comment.
❓ Where does the file go if there is no directory here? What if there's a relative directory?
With OTEL and the new additional volume settings, we want to allow users to set their pgbouncer
log config without breaking OTEL.
(This version does not have a lot of guardrails guiding the user to find a safe setting. We may want to
consider that or punt this idea to a new ticket.)
Checklist:
Type of Changes:
What is the current behavior (link to any open issues here)?
Users can set any logfile config for pgbouncer, which might interfere with our OTEL
What is the new behavior (if this is a feature change)?
We now restrict users to only set a logfile with the
.logsuffix; and we get the config to handle the OTEL-related logicTODO: Restrict the logfile to only certain locations?
Other Information:
Issues: [PGO-2565]