diff --git a/internal/collector/pgbouncer.go b/internal/collector/pgbouncer.go index 785b2b187..a72c60e89 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 34f2ccf32..f7cc85d5d 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 e3ccfe068..8b74f20a6 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,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() { diff --git a/internal/controller/postgrescluster/pgbouncer_test.go b/internal/controller/postgrescluster/pgbouncer_test.go index b17e783d8..26f6637ea 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 1c08e9480..8f924ae92 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 8eed54a3b..613f0dbae 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 dd59a1a33..fe0287931 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