diff --git a/config/crd/bases/postgres-operator.crunchydata.com_postgresclusters.yaml b/config/crd/bases/postgres-operator.crunchydata.com_postgresclusters.yaml index d3e8d3db8b..104bc419cc 100644 --- a/config/crd/bases/postgres-operator.crunchydata.com_postgresclusters.yaml +++ b/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 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 46b66cdbf5..7543b318c2 100644 --- a/internal/controller/postgrescluster/pgbouncer.go +++ b/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,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} @@ -66,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")) @@ -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,7 +485,13 @@ 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 @@ -469,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 @@ -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() { diff --git a/internal/controller/postgrescluster/pgbouncer_test.go b/internal/controller/postgrescluster/pgbouncer_test.go index e6df4fbab8..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) @@ -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) 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 diff --git a/internal/testing/validation/pgbouncer_test.go b/internal/testing/validation/pgbouncer_test.go new file mode 100644 index 0000000000..3e6345a656 --- /dev/null +++ b/internal/testing/validation/pgbouncer_test.go @@ -0,0 +1,168 @@ +// Copyright 2021 - 2025 Crunchy Data Solutions, Inc. +// +// SPDX-License-Identifier: Apache-2.0 + +package validation + +import ( + "context" + "testing" + + "gotest.tools/v3/assert" + apierrors "k8s.io/apimachinery/pkg/api/errors" + "sigs.k8s.io/controller-runtime/pkg/client" + + "github.com/crunchydata/postgres-operator/internal/testing/require" + v1 "github.com/crunchydata/postgres-operator/pkg/apis/postgres-operator.crunchydata.com/v1" +) + +func TestV1PGBouncerLogging(t *testing.T) { + ctx := context.Background() + cc := require.Kubernetes(t) + t.Parallel() + + namespace := require.Namespace(t, cc) + + base := v1.NewPostgresCluster() + base.Namespace = namespace.Name + base.Name = "pgbouncer-logging" + // required fields + require.UnmarshalInto(t, &base.Spec, `{ + postgresVersion: 16, + instances: [{ + dataVolumeClaimSpec: { + accessModes: [ReadWriteOnce], + resources: { requests: { storage: 1Mi } }, + }, + }], + }`) + + assert.NilError(t, cc.Create(ctx, base.DeepCopy(), client.DryRunAll), + "expected this base to be valid") + + t.Run("Can set logging on tmp with .log", func(t *testing.T) { + tmp := base.DeepCopy() + + require.UnmarshalInto(t, &tmp.Spec.Proxy, `{ + pgBouncer: { + config: { + global: { + logfile: "/tmp/logs/pgbouncer/log.log" + } + } + } + }`) + assert.NilError(t, cc.Create(ctx, base.DeepCopy(), client.DryRunAll), + "expected this option to be valid") + }) + + t.Run("Cannot set logging on tmp without .log", func(t *testing.T) { + tmp := base.DeepCopy() + + require.UnmarshalInto(t, &tmp.Spec.Proxy, `{ + pgBouncer: { + config: { + global: { + logfile: "/tmp/logs/pgbouncer/log.txt" + } + } + } + }`) + + err := cc.Create(ctx, tmp.DeepCopy(), client.DryRunAll) + assert.Assert(t, apierrors.IsInvalid(err)) + assert.ErrorContains(t, err, "logfile config must end with '.log'") + }) + + t.Run("Cannot set logging on tmp without correct subdir", func(t *testing.T) { + tmp := base.DeepCopy() + + require.UnmarshalInto(t, &tmp.Spec.Proxy, `{ + pgBouncer: { + config: { + global: { + logfile: "/tmp/logs/log.log" + } + } + } + }`) + + err := cc.Create(ctx, tmp.DeepCopy(), client.DryRunAll) + assert.Assert(t, apierrors.IsInvalid(err)) + assert.ErrorContains(t, err, "logfile destination is restricted to '/tmp/logs/pgbouncer/' or an existing additional volume") + + require.UnmarshalInto(t, &tmp.Spec.Proxy, `{ + pgBouncer: { + config: { + global: { + logfile: "/tmp/pgbouncer/log.log" + } + } + } + }`) + + err = cc.Create(ctx, tmp.DeepCopy(), client.DryRunAll) + assert.Assert(t, apierrors.IsInvalid(err)) + assert.ErrorContains(t, err, "logfile destination is restricted to '/tmp/logs/pgbouncer/' or an existing additional volume") + }) + + t.Run("Cannot set logging on volumes that don't exist", func(t *testing.T) { + vol := base.DeepCopy() + + require.UnmarshalInto(t, &vol.Spec.Proxy, `{ + pgBouncer: { + config: { + global: { + logfile: "/volumes/logging/log.log" + } + } + } + }`) + + err := cc.Create(ctx, vol.DeepCopy(), client.DryRunAll) + assert.Assert(t, apierrors.IsInvalid(err)) + assert.ErrorContains(t, err, "logfile destination is restricted to '/tmp/logs/pgbouncer/' or an existing additional volume") + }) + + t.Run("Cannot set logging elsewhere", func(t *testing.T) { + vol := base.DeepCopy() + + require.UnmarshalInto(t, &vol.Spec.Proxy, `{ + pgBouncer: { + config: { + global: { + logfile: "/var/log.log" + } + } + } + }`) + + err := cc.Create(ctx, vol.DeepCopy(), client.DryRunAll) + assert.Assert(t, apierrors.IsInvalid(err)) + assert.ErrorContains(t, err, "logfile destination is restricted to '/tmp/logs/pgbouncer/' or an existing additional volume") + }) + + t.Run("Can set logging on volumes that exist", func(t *testing.T) { + vol := base.DeepCopy() + + require.UnmarshalInto(t, &vol.Spec.Proxy, `{ + pgBouncer: { + config: { + global: { + logfile: "/volumes/logging/log.log" + } + }, + volumes: { + additional: [ + { + name: logging, + claimName: required-1 + }] + } + } + }`) + + assert.NilError(t, cc.Create(ctx, vol.DeepCopy(), client.DryRunAll), + "expected this option to be valid") + }) +} diff --git a/internal/util/pod_security.go b/internal/util/pod_security.go new file mode 100644 index 0000000000..5de03c06a5 --- /dev/null +++ b/internal/util/pod_security.go @@ -0,0 +1,40 @@ +// Copyright 2017 - 2025 Crunchy Data Solutions, Inc. +// +// SPDX-License-Identifier: Apache-2.0 + +package util + +import ( + corev1 "k8s.io/api/core/v1" + + "github.com/crunchydata/postgres-operator/internal/initialize" +) + +// PodSecurityContext returns a v1.PodSecurityContext for cluster that can write +// to PersistentVolumes. +// This func sets the supplmental groups and fsGgroup if present. +// fsGroup should not be present in OpenShift environments +func PodSecurityContext(fsgroup int64, supplementalGroups []int64) *corev1.PodSecurityContext { + psc := initialize.PodSecurityContext() + + // Use the specified supplementary groups except for root. The CRD has + // similar validation, but we should never emit a PodSpec with that group. + // - https://docs.k8s.io/concepts/security/pod-security-standards/ + for i := range supplementalGroups { + if gid := supplementalGroups[i]; gid > 0 { + psc.SupplementalGroups = append(psc.SupplementalGroups, gid) + } + } + + // OpenShift assigns a filesystem group based on a SecurityContextConstraint. + // Otherwise, set a filesystem group so PostgreSQL can write to files + // regardless of the UID or GID of a container. + // - https://cloud.redhat.com/blog/a-guide-to-openshift-and-uids + // - https://docs.k8s.io/tasks/configure-pod-container/security-context/ + // - https://docs.openshift.com/container-platform/4.8/authentication/managing-security-context-constraints.html + if fsgroup > 0 { + psc.FSGroup = initialize.Int64(fsgroup) + } + + return psc +} diff --git a/internal/util/pod_security_test.go b/internal/util/pod_security_test.go new file mode 100644 index 0000000000..e759e6902a --- /dev/null +++ b/internal/util/pod_security_test.go @@ -0,0 +1,52 @@ +// Copyright 2017 - 2025 Crunchy Data Solutions, Inc. +// +// SPDX-License-Identifier: Apache-2.0 + +package util + +import ( + "testing" + + "gotest.tools/v3/assert" + + "github.com/crunchydata/postgres-operator/internal/testing/cmp" +) + +func TestPodSecurityContext(t *testing.T) { + t.Run("Non-Openshift", func(t *testing.T) { + assert.Assert(t, cmp.MarshalMatches(PodSecurityContext(2, []int64{}), ` +fsGroup: 2 +fsGroupChangePolicy: OnRootMismatch + `)) + + supplementalGroups := []int64{3, 4} + assert.Assert(t, cmp.MarshalMatches(PodSecurityContext(26, supplementalGroups), ` +fsGroup: 26 +fsGroupChangePolicy: OnRootMismatch +supplementalGroups: +- 3 +- 4 + `)) + }) + + t.Run("OpenShift", func(t *testing.T) { + assert.Assert(t, cmp.MarshalMatches(PodSecurityContext(0, []int64{}), + `fsGroupChangePolicy: OnRootMismatch`)) + + supplementalGroups := []int64{3, 4} + assert.Assert(t, cmp.MarshalMatches(PodSecurityContext(0, supplementalGroups), ` +fsGroupChangePolicy: OnRootMismatch +supplementalGroups: +- 3 +- 4 + `)) + }) + + t.Run("NoRootGID", func(t *testing.T) { + supplementalGroups := []int64{999, 0, 100, 0} + assert.DeepEqual(t, []int64{999, 100}, PodSecurityContext(2, supplementalGroups).SupplementalGroups) + + supplementalGroups = []int64{0} + assert.Assert(t, PodSecurityContext(2, supplementalGroups).SupplementalGroups == nil) + }) +} diff --git a/pkg/apis/postgres-operator.crunchydata.com/v1/pgbouncer_types.go b/pkg/apis/postgres-operator.crunchydata.com/v1/pgbouncer_types.go new file mode 100644 index 0000000000..45c1269c14 --- /dev/null +++ b/pkg/apis/postgres-operator.crunchydata.com/v1/pgbouncer_types.go @@ -0,0 +1,15 @@ +// Copyright 2021 - 2025 Crunchy Data Solutions, Inc. +// +// SPDX-License-Identifier: Apache-2.0 + +package v1 + +import ( + "github.com/crunchydata/postgres-operator/pkg/apis/postgres-operator.crunchydata.com/v1beta1" +) + +// PGBouncerPodSpec defines the desired state of a PgBouncer connection pooler. +// +kubebuilder:validation:XValidation: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))`,message=`logfile destination is restricted to '/tmp/logs/pgbouncer/' or an existing additional volume` +type PGBouncerPodSpec struct { + v1beta1.PGBouncerPodSpec `json:",inline"` +} diff --git a/pkg/apis/postgres-operator.crunchydata.com/v1/postgrescluster_types.go b/pkg/apis/postgres-operator.crunchydata.com/v1/postgrescluster_types.go index e9778b93bb..df70cd5cb2 100644 --- a/pkg/apis/postgres-operator.crunchydata.com/v1/postgrescluster_types.go +++ b/pkg/apis/postgres-operator.crunchydata.com/v1/postgrescluster_types.go @@ -598,7 +598,7 @@ type PostgresInstanceSetStatus struct { type PostgresProxySpec struct { // Defines a PgBouncer proxy and connection pooler. - PGBouncer *v1beta1.PGBouncerPodSpec `json:"pgBouncer"` + PGBouncer *PGBouncerPodSpec `json:"pgBouncer"` } // Default sets the defaults for any proxies that are set. diff --git a/pkg/apis/postgres-operator.crunchydata.com/v1/zz_generated.deepcopy.go b/pkg/apis/postgres-operator.crunchydata.com/v1/zz_generated.deepcopy.go index b0916d8cce..5ff219c04c 100644 --- a/pkg/apis/postgres-operator.crunchydata.com/v1/zz_generated.deepcopy.go +++ b/pkg/apis/postgres-operator.crunchydata.com/v1/zz_generated.deepcopy.go @@ -182,6 +182,22 @@ func (in *MonitoringStatus) DeepCopy() *MonitoringStatus { return out } +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *PGBouncerPodSpec) DeepCopyInto(out *PGBouncerPodSpec) { + *out = *in + in.PGBouncerPodSpec.DeepCopyInto(&out.PGBouncerPodSpec) +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new PGBouncerPodSpec. +func (in *PGBouncerPodSpec) DeepCopy() *PGBouncerPodSpec { + if in == nil { + return nil + } + out := new(PGBouncerPodSpec) + in.DeepCopyInto(out) + return out +} + // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *PostgresCluster) DeepCopyInto(out *PostgresCluster) { *out = *in @@ -587,7 +603,7 @@ func (in *PostgresProxySpec) DeepCopyInto(out *PostgresProxySpec) { *out = *in if in.PGBouncer != nil { in, out := &in.PGBouncer, &out.PGBouncer - *out = new(v1beta1.PGBouncerPodSpec) + *out = new(PGBouncerPodSpec) (*in).DeepCopyInto(*out) } } diff --git a/pkg/apis/postgres-operator.crunchydata.com/v1beta1/pgbouncer_types.go b/pkg/apis/postgres-operator.crunchydata.com/v1beta1/pgbouncer_types.go index 49e713c17a..72244bc64b 100644 --- a/pkg/apis/postgres-operator.crunchydata.com/v1beta1/pgbouncer_types.go +++ b/pkg/apis/postgres-operator.crunchydata.com/v1beta1/pgbouncer_types.go @@ -26,7 +26,13 @@ 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'` + // See also XValidation rule on v1 PostgresProxySpec + // // +optional + // +mapType=granular Global map[string]string `json:"global,omitempty"` // PgBouncer database definitions. The key is the database requested by a