Validate and use pgbouncer logfile config

config/crd/bases/postgres-operator.crunchydata.com_postgresclusters.yaml

@@ -14376,6 +14376,10 @@ spec:
                               Settings that apply to the entire PgBouncer process.
                               More info: https://www.pgbouncer.org/config.html
                             type: object
+                            x-kubernetes-map-type: granular
+                            x-kubernetes-validations:
+                            - message: logfile config must end with '.log'
+                              rule: '!has(self.logfile) || self.logfile.endsWith(''.log'')'
                           users:
                             additionalProperties:
                               type: string
@@ -16405,6 +16409,12 @@ spec:
                             x-kubernetes-list-type: map
                         type: object
                     type: object
+                    x-kubernetes-validations:
+                    - message: logfile destination is restricted to '/tmp/logs/pgbouncer/'
+                        or an existing additional volume
+                      rule: '!has(self.config) || !has(self.config.global) || !has(self.config.global.logfile)
+                        || self.config.global.logfile.startsWith(''/tmp/logs/pgbouncer/'')
+                        || self.volumes.additional.exists(x, self.config.global.logfile.startsWith("/volumes/"+x.name))'
                 required:
                 - pgBouncer
                 type: object
@@ -33304,6 +33314,10 @@ spec:
                               Settings that apply to the entire PgBouncer process.
                               More info: https://www.pgbouncer.org/config.html
                             type: object
+                            x-kubernetes-map-type: granular
+                            x-kubernetes-validations:
+                            - message: logfile config must end with '.log'
+                              rule: '!has(self.logfile) || self.logfile.endsWith(''.log'')'
                           users:
                             additionalProperties:
                               type: string

internal/collector/pgbouncer.go

@@ -9,6 +9,7 @@ import (
 	_ "embed"
 	"encoding/json"
 	"fmt"
+	"path/filepath"
 	"slices"
 	"strconv"
 
@@ -29,7 +30,7 @@ const PGBouncerPostRotateScript = "pkill -HUP --exact pgbouncer"
 // NewConfigForPgBouncerPod creates a config for the OTel collector container
 // that runs as a sidecar in the pgBouncer Pod
 func NewConfigForPgBouncerPod(
-	ctx context.Context, cluster *v1beta1.PostgresCluster, sqlQueryUsername string,
+	ctx context.Context, cluster *v1beta1.PostgresCluster, sqlQueryUsername, logfile string,
 ) *Config {
 	if cluster.Spec.Proxy == nil || cluster.Spec.Proxy.PGBouncer == nil {
 		// pgBouncer is disabled; return nil
@@ -38,7 +39,7 @@ func NewConfigForPgBouncerPod(
 
 	config := NewConfig(cluster.Spec.Instrumentation)
 
-	EnablePgBouncerLogging(ctx, cluster, config)
+	EnablePgBouncerLogging(ctx, cluster, config, logfile)
 	EnablePgBouncerMetrics(ctx, cluster, config, sqlQueryUsername)
 
 	return config
@@ -49,14 +50,15 @@ func NewConfigForPgBouncerPod(
 func EnablePgBouncerLogging(ctx context.Context,
 	inCluster *v1beta1.PostgresCluster,
 	outConfig *Config,
+	logfile string,
 ) {
 	var spec *v1beta1.InstrumentationLogsSpec
 	if inCluster != nil && inCluster.Spec.Instrumentation != nil {
 		spec = inCluster.Spec.Instrumentation.Logs
 	}
 
 	if OpenTelemetryLogsEnabled(ctx, inCluster) {
-		directory := naming.PGBouncerLogPath
+		directory := filepath.Dir(logfile)
 
 		// Keep track of what log records and files have been processed.
 		// Use a subdirectory of the logs directory to stay within the same failure domain.

internal/collector/pgbouncer_test.go

@@ -11,6 +11,7 @@ import (
 	"gotest.tools/v3/assert"
 
 	"github.com/crunchydata/postgres-operator/internal/feature"
+	"github.com/crunchydata/postgres-operator/internal/naming"
 	"github.com/crunchydata/postgres-operator/internal/testing/require"
 	"github.com/crunchydata/postgres-operator/pkg/apis/postgres-operator.crunchydata.com/v1beta1"
 )
@@ -28,7 +29,7 @@ func TestEnablePgBouncerLogging(t *testing.T) {
 		require.UnmarshalInto(t, &cluster.Spec, `{
 			instrumentation: {}
 		}`)
-		EnablePgBouncerLogging(ctx, cluster, config)
+		EnablePgBouncerLogging(ctx, cluster, config, naming.PGBouncerFullLogPath)
 
 		result, err := config.ToYAML()
 		assert.NilError(t, err)
@@ -127,7 +128,7 @@ service:
 		cluster := new(v1beta1.PostgresCluster)
 		cluster.Spec.Instrumentation = testInstrumentationSpec()
 
-		EnablePgBouncerLogging(ctx, cluster, config)
+		EnablePgBouncerLogging(ctx, cluster, config, naming.PGBouncerFullLogPath)
 
 		result, err := config.ToYAML()
 		assert.NilError(t, err)

internal/controller/postgrescluster/pgbouncer.go

@@ -25,6 +25,7 @@ import (
 	"github.com/crunchydata/postgres-operator/internal/pgbouncer"
 	"github.com/crunchydata/postgres-operator/internal/pki"
 	"github.com/crunchydata/postgres-operator/internal/postgres"
+	"github.com/crunchydata/postgres-operator/internal/util"
 	"github.com/crunchydata/postgres-operator/pkg/apis/postgres-operator.crunchydata.com/v1beta1"
 )
 
@@ -43,12 +44,13 @@ func (r *Reconciler) reconcilePGBouncer(
 	if err == nil {
 		secret, err = r.reconcilePGBouncerSecret(ctx, cluster, root, service)
 	}
+	logfile := setPGBouncerLogfile(cluster)
 	if err == nil {
-		config := collector.NewConfigForPgBouncerPod(ctx, cluster, pgbouncer.PostgresqlUser)
-		configmap, err = r.reconcilePGBouncerConfigMap(ctx, cluster, config)
+		config := collector.NewConfigForPgBouncerPod(ctx, cluster, pgbouncer.PostgresqlUser, logfile)
+		configmap, err = r.reconcilePGBouncerConfigMap(ctx, cluster, config, logfile)
 	}
 	if err == nil {
-		err = r.reconcilePGBouncerDeployment(ctx, cluster, primaryCertificate, configmap, secret)
+		err = r.reconcilePGBouncerDeployment(ctx, cluster, primaryCertificate, configmap, secret, logfile)
 	}
 	if err == nil {
 		err = r.reconcilePGBouncerPodDisruptionBudget(ctx, cluster)
@@ -59,13 +61,34 @@ func (r *Reconciler) reconcilePGBouncer(
 	return err
 }
 
+// setPGBouncerLogfile retrieves the logfile config if present in the user config.
+// If not present, set to the OTEL default.
+// If OTEL is not enabled, we do not use this value.
+// TODO: Check INI config files specified on the cluster
+func setPGBouncerLogfile(cluster *v1beta1.PostgresCluster) string {
+	logfile := naming.PGBouncerFullLogPath
+
+	if cluster.Spec.Proxy == nil || cluster.Spec.Proxy.PGBouncer == nil {
+		return ""
+	}
+
+	if cluster.Spec.Proxy.PGBouncer.Config.Global != nil {
+		if dest, ok := cluster.Spec.Proxy.PGBouncer.Config.Global["logfile"]; ok {
+			logfile = dest
+		}
+	}
+
+	return logfile
+}
+
 // +kubebuilder:rbac:groups="",resources="configmaps",verbs={get}
 // +kubebuilder:rbac:groups="",resources="configmaps",verbs={create,delete,patch}
 
 // reconcilePGBouncerConfigMap writes the ConfigMap for a PgBouncer Pod.
 func (r *Reconciler) reconcilePGBouncerConfigMap(
 	ctx context.Context, cluster *v1beta1.PostgresCluster,
 	otelConfig *collector.Config,
+	logfile string,
 ) (*corev1.ConfigMap, error) {
 	configmap := &corev1.ConfigMap{ObjectMeta: naming.ClusterPGBouncer(cluster)}
 	configmap.SetGroupVersionKind(corev1.SchemeGroupVersion.WithKind("ConfigMap"))
@@ -104,7 +127,7 @@ func (r *Reconciler) reconcilePGBouncerConfigMap(
 	// If OTel logging is enabled, add logrotate config
 	if err == nil && collector.OpenTelemetryLogsEnabled(ctx, cluster) {
 		logrotateConfig := collector.LogrotateConfig{
-			LogFiles:         []string{naming.PGBouncerFullLogPath},
+			LogFiles:         []string{logfile},
 			PostrotateScript: collector.PGBouncerPostRotateScript,
 		}
 		collector.AddLogrotateConfigs(ctx, cluster.Spec.Instrumentation, configmap,
@@ -369,6 +392,7 @@ func (r *Reconciler) generatePGBouncerDeployment(
 	ctx context.Context, cluster *v1beta1.PostgresCluster,
 	primaryCertificate *corev1.SecretProjection,
 	configmap *corev1.ConfigMap, secret *corev1.Secret,
+	logfile string,
 ) (*appsv1.Deployment, bool, error) {
 	deploy := &appsv1.Deployment{ObjectMeta: naming.ClusterPGBouncer(cluster)}
 	deploy.SetGroupVersionKind(appsv1.SchemeGroupVersion.WithKind("Deployment"))
@@ -461,15 +485,21 @@ func (r *Reconciler) generatePGBouncerDeployment(
 	// Do not add environment variables describing services in this namespace.
 	deploy.Spec.Template.Spec.EnableServiceLinks = initialize.Bool(false)
 
-	deploy.Spec.Template.Spec.SecurityContext = initialize.PodSecurityContext()
+	fsGroup := 2
+	if initialize.FromPointer(cluster.Spec.OpenShift) {
+		fsGroup = 0
+	}
+	deploy.Spec.Template.Spec.SecurityContext = util.PodSecurityContext(int64(fsGroup),
+		cluster.Spec.SupplementalGroups,
+	)
 
 	// set the image pull secrets, if any exist
 	deploy.Spec.Template.Spec.ImagePullSecrets = cluster.Spec.ImagePullSecrets
 
 	err := errors.WithStack(r.setControllerReference(cluster, deploy))
 
 	if err == nil {
-		pgbouncer.Pod(ctx, cluster, configmap, primaryCertificate, secret, &deploy.Spec.Template)
+		pgbouncer.Pod(ctx, cluster, configmap, primaryCertificate, secret, &deploy.Spec.Template, logfile)
 	}
 
 	// Add tmp directory and volume for log files
@@ -496,9 +526,10 @@ func (r *Reconciler) reconcilePGBouncerDeployment(
 	ctx context.Context, cluster *v1beta1.PostgresCluster,
 	primaryCertificate *corev1.SecretProjection,
 	configmap *corev1.ConfigMap, secret *corev1.Secret,
+	logfile string,
 ) error {
 	deploy, specified, err := r.generatePGBouncerDeployment(
-		ctx, cluster, primaryCertificate, configmap, secret)
+		ctx, cluster, primaryCertificate, configmap, secret, logfile)
 
 	// Set observations whether the deployment exists or not.
 	defer func() {

internal/controller/postgrescluster/pgbouncer_test.go

@@ -382,7 +382,7 @@ func TestGeneratePGBouncerDeployment(t *testing.T) {
 			cluster := cluster.DeepCopy()
 			cluster.Spec.Proxy = spec
 
-			deploy, specified, err := reconciler.generatePGBouncerDeployment(ctx, cluster, nil, nil, nil)
+			deploy, specified, err := reconciler.generatePGBouncerDeployment(ctx, cluster, nil, nil, nil, "")
 			assert.NilError(t, err)
 			assert.Assert(t, !specified)
 
@@ -415,7 +415,7 @@ namespace: ns3
 		}
 
 		deploy, specified, err := reconciler.generatePGBouncerDeployment(
-			ctx, cluster, primary, configmap, secret)
+			ctx, cluster, primary, configmap, secret, "")
 		assert.NilError(t, err)
 		assert.Assert(t, specified)
 
@@ -456,7 +456,7 @@ namespace: ns3
 
 	t.Run("PodSpec", func(t *testing.T) {
 		deploy, specified, err := reconciler.generatePGBouncerDeployment(
-			ctx, cluster, primary, configmap, secret)
+			ctx, cluster, primary, configmap, secret, "")
 		assert.NilError(t, err)
 		assert.Assert(t, specified)
 
@@ -478,6 +478,7 @@ containers: null
 enableServiceLinks: false
 restartPolicy: Always
 securityContext:
+  fsGroup: 2
   fsGroupChangePolicy: OnRootMismatch
 shareProcessNamespace: true
 topologySpreadConstraints:
@@ -502,7 +503,7 @@ topologySpreadConstraints:
 			cluster.Spec.DisableDefaultPodScheduling = initialize.Bool(true)
 
 			deploy, specified, err := reconciler.generatePGBouncerDeployment(
-				ctx, cluster, primary, configmap, secret)
+				ctx, cluster, primary, configmap, secret, "")
 			assert.NilError(t, err)
 			assert.Assert(t, specified)
 
@@ -520,7 +521,7 @@ topologySpreadConstraints:
 		}
 
 		deploy, specified, err := reconciler.generatePGBouncerDeployment(
-			ctx, cluster, primary, configmap, secret)
+			ctx, cluster, primary, configmap, secret, "")
 
 		assert.NilError(t, err)
 		assert.Assert(t, specified)

internal/pgbouncer/config.go

@@ -127,9 +127,13 @@ func clusterINI(ctx context.Context, cluster *v1beta1.PostgresCluster) string {
 		"unix_socket_dir": "",
 	}
 
+	// Override the above with any specified settings.
+	maps.Copy(global, cluster.Spec.Proxy.PGBouncer.Config.Global)
+
 	// If OpenTelemetryLogs feature is enabled, enable logging to file
-	if collector.OpenTelemetryLogsEnabled(ctx, cluster) {
-		global["logfile"] = naming.PGBouncerLogPath + "/pgbouncer.log"
+	// if not otherwise set
+	if _, ok := global["logfile"]; !ok && collector.OpenTelemetryLogsEnabled(ctx, cluster) {
+		global["logfile"] = naming.PGBouncerFullLogPath
 	}
 
 	// When OTel metrics are enabled, allow pgBouncer's postgres user
@@ -138,9 +142,6 @@ func clusterINI(ctx context.Context, cluster *v1beta1.PostgresCluster) string {
 		global["stats_users"] = PostgresqlUser
 	}
 
-	// Override the above with any specified settings.
-	maps.Copy(global, cluster.Spec.Proxy.PGBouncer.Config.Global)
-
 	// Prevent the user from bypassing the main configuration file.
 	global["conffile"] = iniFileAbsolutePath
 

internal/pgbouncer/reconcile.go

@@ -6,6 +6,7 @@ package pgbouncer
 
 import (
 	"context"
+	"path/filepath"
 
 	"github.com/pkg/errors"
 	corev1 "k8s.io/api/core/v1"
@@ -19,6 +20,7 @@ import (
 	"github.com/crunchydata/postgres-operator/internal/pki"
 	"github.com/crunchydata/postgres-operator/internal/postgres"
 	passwd "github.com/crunchydata/postgres-operator/internal/postgres/password"
+	"github.com/crunchydata/postgres-operator/internal/shell"
 	"github.com/crunchydata/postgres-operator/internal/util"
 	"github.com/crunchydata/postgres-operator/pkg/apis/postgres-operator.crunchydata.com/v1beta1"
 )
@@ -128,6 +130,7 @@ func Pod(
 	inPostgreSQLCertificate *corev1.SecretProjection,
 	inSecret *corev1.Secret,
 	template *corev1.PodTemplateSpec,
+	logfile string,
 ) {
 	if inCluster.Spec.Proxy == nil || inCluster.Spec.Proxy.PGBouncer == nil {
 		// PgBouncer is disabled; there is nothing to do.
@@ -146,10 +149,20 @@ func Pod(
 		),
 	}
 
+	mkdirCommand := ""
+	// filepath.Dir will return an "" when given an ""
+	logPath := filepath.Dir(logfile)
+	// If the logpath is `/tmp`, we don't need to worry about creating/chmoding it.
+	// Otherwise, use `MakeDirectories` to create/chmod that specific directory,
+	// without worrying about parent directories.
+	if logfile != "" && logPath != "/tmp" {
+		mkdirCommand = shell.MakeDirectories(logPath, logPath) + "; "
+	}
+
 	container := corev1.Container{
 		Name: naming.ContainerPGBouncer,
 
-		Command:         []string{"pgbouncer", iniFileAbsolutePath},
+		Command:         []string{"sh", "-c", "--", mkdirCommand + `exec "$@"`, "--", "pgbouncer", iniFileAbsolutePath},
 		Image:           config.PGBouncerContainerImage(inCluster),
 		ImagePullPolicy: inCluster.Spec.ImagePullPolicy,
 		Resources:       inCluster.Spec.Proxy.PGBouncer.Resources,