hold

Author: benjaminjb

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

@@ -14375,7 +14375,13 @@ spec:
                             description: |-
                               Settings that apply to the entire PgBouncer process.
                               More info: https://www.pgbouncer.org/config.html
+
+                              # Logging
                             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
@@ -33300,7 +33306,13 @@ spec:
                             description: |-
                               Settings that apply to the entire PgBouncer process.
                               More info: https://www.pgbouncer.org/config.html
+
+                              # Logging
                             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

@@ -44,8 +44,9 @@ func (r *Reconciler) reconcilePGBouncer(
 		secret, err = r.reconcilePGBouncerSecret(ctx, cluster, root, service)
 	}
 	if err == nil {
-		config := collector.NewConfigForPgBouncerPod(ctx, cluster, pgbouncer.PostgresqlUser)
-		configmap, err = r.reconcilePGBouncerConfigMap(ctx, cluster, config)
+		logfile := setPGBouncerLogfile(ctx, cluster)
+		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)
@@ -59,13 +60,28 @@ 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(ctx context.Context, cluster *v1beta1.PostgresCluster) string {
+	logfile := naming.PGBouncerFullLogPath
+
+	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 +120,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,

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

@@ -37,7 +37,8 @@ func ConfigMap(
 	initialize.Map(&outConfigMap.Data)
 
 	outConfigMap.Data[emptyConfigMapKey] = ""
-	outConfigMap.Data[iniFileConfigMapKey] = clusterINI(ctx, inCluster)
+	clusterINI := clusterINI(ctx, inCluster)
+	outConfigMap.Data[iniFileConfigMapKey] = clusterINI
 }
 
 // Secret populates the PgBouncer Secret.

pkg/apis/postgres-operator.crunchydata.com/v1beta1/pgbouncer_types.go

@@ -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'`
+	//
 	// +optional
+	// +mapType=granular
 	Global map[string]string `json:"global,omitempty"`
 
 	// PgBouncer database definitions. The key is the database requested by a