Handle user-set pgBouncer logfile for OTEL, mkdir (CrunchyData#4278)

* 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.

This PR also has the pgBouncer mkdir the appropriate directory
if given that configuration.

Issues: [PGO-2565]

Author: benjaminjb

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,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,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"))
@@ -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,7 +499,7 @@ 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
@@ -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() {

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)

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,