From eb5ea9bba192924478bab49e31a8f572f6a598a5 Mon Sep 17 00:00:00 2001 From: benjaminjb Date: Tue, 9 Sep 2025 16:26:16 -0500 Subject: [PATCH 1/2] Handle user-set pgBouncer logfile for OTEL pgBouncer allows users to set the log destination; when OTEL is enabled, we assume that destination is the default that we set. This PR patches that hole by setting OTEL to use the either the default or the user-set log destination. Issues: [PGO-2565] --- internal/collector/pgbouncer.go | 8 +- internal/collector/pgbouncer_test.go | 5 +- .../controller/postgrescluster/pgbouncer.go | 40 ++- .../postgrescluster/pgbouncer_test.go | 10 +- internal/pgbouncer/config.go | 11 +- internal/pgbouncer/reconcile.go | 15 +- internal/pgbouncer/reconcile_test.go | 240 +++++++++++++++++- 7 files changed, 304 insertions(+), 25 deletions(-) diff --git a/internal/collector/pgbouncer.go b/internal/collector/pgbouncer.go index 785b2b187e..a72c60e898 100644 --- a/internal/collector/pgbouncer.go +++ b/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,6 +50,7 @@ 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 { @@ -56,7 +58,7 @@ func EnablePgBouncerLogging(ctx context.Context, } 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. diff --git a/internal/collector/pgbouncer_test.go b/internal/collector/pgbouncer_test.go index 34f2ccf328..f7cc85d5d4 100644 --- a/internal/collector/pgbouncer_test.go +++ b/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) diff --git a/internal/controller/postgrescluster/pgbouncer.go b/internal/controller/postgrescluster/pgbouncer.go index e3ccfe0683..0a7bbb74ca 100644 --- a/internal/controller/postgrescluster/pgbouncer.go +++ b/internal/controller/postgrescluster/pgbouncer.go @@ -44,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) @@ -60,6 +61,26 @@ 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} @@ -67,6 +88,7 @@ func (r *Reconciler) reconcilePGBouncer( 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")) @@ -105,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, @@ -370,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")) @@ -476,15 +499,15 @@ func (r *Reconciler) generatePGBouncerDeployment( 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 AddTMPEmptyDir(&deploy.Spec.Template) // mount additional volumes to the pgbouncer containers - if volumes := cluster.Spec.Proxy.PGBouncer.Volumes; err == nil && volumes != nil && len(volumes.Additional) > 0 { - missingContainers := util.AddAdditionalVolumesAndMounts(&deploy.Spec.Template.Spec, volumes.Additional) + if err == nil && cluster.Spec.Proxy.PGBouncer.Volumes != nil && len(cluster.Spec.Proxy.PGBouncer.Volumes.Additional) > 0 { + missingContainers := util.AddAdditionalVolumesAndMounts(&deploy.Spec.Template.Spec, cluster.Spec.Proxy.PGBouncer.Volumes.Additional) if len(missingContainers) > 0 { r.Recorder.Eventf(cluster, corev1.EventTypeWarning, "SpecifiedContainerNotFound", @@ -503,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() { diff --git a/internal/controller/postgrescluster/pgbouncer_test.go b/internal/controller/postgrescluster/pgbouncer_test.go index b17e783d8f..26f6637ead 100644 --- a/internal/controller/postgrescluster/pgbouncer_test.go +++ b/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) @@ -503,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) @@ -521,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) diff --git a/internal/pgbouncer/config.go b/internal/pgbouncer/config.go index 1c08e94803..8f924ae928 100644 --- a/internal/pgbouncer/config.go +++ b/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 diff --git a/internal/pgbouncer/reconcile.go b/internal/pgbouncer/reconcile.go index 8eed54a3b6..613f0dbae4 100644 --- a/internal/pgbouncer/reconcile.go +++ b/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, diff --git a/internal/pgbouncer/reconcile_test.go b/internal/pgbouncer/reconcile_test.go index dd59a1a337..fe0287931a 100644 --- a/internal/pgbouncer/reconcile_test.go +++ b/internal/pgbouncer/reconcile_test.go @@ -149,8 +149,9 @@ func TestPod(t *testing.T) { primaryCertificate := new(corev1.SecretProjection) secret := new(corev1.Secret) template := new(corev1.PodTemplateSpec) + logfile := "" - call := func() { Pod(ctx, cluster, configMap, primaryCertificate, secret, template) } + call := func() { Pod(ctx, cluster, configMap, primaryCertificate, secret, template, logfile) } t.Run("Disabled", func(t *testing.T) { before := template.DeepCopy() @@ -170,6 +171,11 @@ func TestPod(t *testing.T) { assert.Assert(t, cmp.MarshalMatches(template.Spec, ` containers: - command: + - sh + - -c + - -- + - exec "$@" + - -- - pgbouncer - /etc/pgbouncer/~postgres-operator.ini name: pgbouncer @@ -280,6 +286,230 @@ volumes: assert.Assert(t, cmp.MarshalMatches(template.Spec, ` containers: - command: + - sh + - -c + - -- + - exec "$@" + - -- + - pgbouncer + - /etc/pgbouncer/~postgres-operator.ini + image: image-town + imagePullPolicy: Always + name: pgbouncer + ports: + - containerPort: 5432 + name: pgbouncer + protocol: TCP + resources: + requests: + cpu: 100m + securityContext: + allowPrivilegeEscalation: false + capabilities: + drop: + - ALL + privileged: false + readOnlyRootFilesystem: true + runAsNonRoot: true + seccompProfile: + type: RuntimeDefault + volumeMounts: + - mountPath: /etc/pgbouncer + name: pgbouncer-config + readOnly: true +- command: + - bash + - -ceu + - -- + - |- + monitor() { + exec {fd}<> <(:||:) + while read -r -t 5 -u "${fd}" ||:; do + if [[ "${directory}" -nt "/proc/self/fd/${fd}" ]] && pkill -HUP --exact pgbouncer + then + exec {fd}>&- && exec {fd}<> <(:||:) + stat --format='Loaded configuration dated %y' "${directory}" + fi + done + }; export directory="$1"; export -f monitor; exec -a "$0" bash -ceu monitor + - pgbouncer-config + - /etc/pgbouncer + image: image-town + imagePullPolicy: Always + name: pgbouncer-config + resources: + limits: + cpu: 5m + memory: 16Mi + securityContext: + allowPrivilegeEscalation: false + capabilities: + drop: + - ALL + privileged: false + readOnlyRootFilesystem: true + runAsNonRoot: true + seccompProfile: + type: RuntimeDefault + volumeMounts: + - mountPath: /etc/pgbouncer + name: pgbouncer-config + readOnly: true +volumes: +- name: pgbouncer-config + projected: + sources: + - configMap: + items: + - key: pgbouncer-empty + path: pgbouncer.ini + - configMap: + items: + - key: pgbouncer.ini + path: ~postgres-operator.ini + - secret: + items: + - key: pgbouncer-users.txt + path: ~postgres-operator/users.txt + - secret: + items: + - key: k1 + path: ~postgres-operator/frontend-tls.crt + - key: k2 + path: ~postgres-operator/frontend-tls.key + name: tls-name + - secret: + items: + - key: ca.crt + path: ~postgres-operator/backend-ca.crt + `)) + }) + + t.Run("WithOtelNoLogSet", func(t *testing.T) { + cluster.Spec.Instrumentation = &v1beta1.InstrumentationSpec{} + logfile = "/tmp/pgbouncer.log" + + call() + + assert.Assert(t, cmp.MarshalMatches(template.Spec, ` +containers: +- command: + - sh + - -c + - -- + - exec "$@" + - -- + - pgbouncer + - /etc/pgbouncer/~postgres-operator.ini + image: image-town + imagePullPolicy: Always + name: pgbouncer + ports: + - containerPort: 5432 + name: pgbouncer + protocol: TCP + resources: + requests: + cpu: 100m + securityContext: + allowPrivilegeEscalation: false + capabilities: + drop: + - ALL + privileged: false + readOnlyRootFilesystem: true + runAsNonRoot: true + seccompProfile: + type: RuntimeDefault + volumeMounts: + - mountPath: /etc/pgbouncer + name: pgbouncer-config + readOnly: true +- command: + - bash + - -ceu + - -- + - |- + monitor() { + exec {fd}<> <(:||:) + while read -r -t 5 -u "${fd}" ||:; do + if [[ "${directory}" -nt "/proc/self/fd/${fd}" ]] && pkill -HUP --exact pgbouncer + then + exec {fd}>&- && exec {fd}<> <(:||:) + stat --format='Loaded configuration dated %y' "${directory}" + fi + done + }; export directory="$1"; export -f monitor; exec -a "$0" bash -ceu monitor + - pgbouncer-config + - /etc/pgbouncer + image: image-town + imagePullPolicy: Always + name: pgbouncer-config + resources: + limits: + cpu: 5m + memory: 16Mi + securityContext: + allowPrivilegeEscalation: false + capabilities: + drop: + - ALL + privileged: false + readOnlyRootFilesystem: true + runAsNonRoot: true + seccompProfile: + type: RuntimeDefault + volumeMounts: + - mountPath: /etc/pgbouncer + name: pgbouncer-config + readOnly: true +volumes: +- name: pgbouncer-config + projected: + sources: + - configMap: + items: + - key: pgbouncer-empty + path: pgbouncer.ini + - configMap: + items: + - key: pgbouncer.ini + path: ~postgres-operator.ini + - secret: + items: + - key: pgbouncer-users.txt + path: ~postgres-operator/users.txt + - secret: + items: + - key: k1 + path: ~postgres-operator/frontend-tls.crt + - key: k2 + path: ~postgres-operator/frontend-tls.key + name: tls-name + - secret: + items: + - key: ca.crt + path: ~postgres-operator/backend-ca.crt + `)) + }) + + t.Run("CustomizationWithLogSet", func(t *testing.T) { + cluster.Spec.Proxy.PGBouncer.Config.Global = map[string]string{ + "logfile": "/volumes/required/mylog.log", + } + logfile = "/volumes/required/mylog.log" + + call() + + assert.Assert(t, cmp.MarshalMatches(template.Spec, ` +containers: +- command: + - sh + - -c + - -- + - mkdir -p '/volumes/required' && { chmod 0775 '/volumes/required' || :; }; exec + "$@" + - -- - pgbouncer - /etc/pgbouncer/~postgres-operator.ini image: image-town @@ -385,11 +615,19 @@ volumes: }, } + // reset logfile from previous test + logfile = "" + call() assert.Assert(t, cmp.MarshalMatches(template.Spec, ` containers: - command: + - sh + - -c + - -- + - exec "$@" + - -- - pgbouncer - /etc/pgbouncer/~postgres-operator.ini image: image-town From ab8c3319480c782faf8f763d065aaa89a3879513 Mon Sep 17 00:00:00 2001 From: benjaminjb Date: Tue, 9 Sep 2025 16:31:06 -0500 Subject: [PATCH 2/2] Bring back in line with main --- internal/controller/postgrescluster/pgbouncer.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/controller/postgrescluster/pgbouncer.go b/internal/controller/postgrescluster/pgbouncer.go index 0a7bbb74ca..8b74f20a67 100644 --- a/internal/controller/postgrescluster/pgbouncer.go +++ b/internal/controller/postgrescluster/pgbouncer.go @@ -506,8 +506,8 @@ func (r *Reconciler) generatePGBouncerDeployment( AddTMPEmptyDir(&deploy.Spec.Template) // mount additional volumes to the pgbouncer containers - if err == nil && cluster.Spec.Proxy.PGBouncer.Volumes != nil && len(cluster.Spec.Proxy.PGBouncer.Volumes.Additional) > 0 { - missingContainers := util.AddAdditionalVolumesAndMounts(&deploy.Spec.Template.Spec, cluster.Spec.Proxy.PGBouncer.Volumes.Additional) + if volumes := cluster.Spec.Proxy.PGBouncer.Volumes; err == nil && volumes != nil && len(volumes.Additional) > 0 { + missingContainers := util.AddAdditionalVolumesAndMounts(&deploy.Spec.Template.Spec, volumes.Additional) if len(missingContainers) > 0 { r.Recorder.Eventf(cluster, corev1.EventTypeWarning, "SpecifiedContainerNotFound",