Skip to content

Commit 937ee38

Browse files
benjaminjbbenjaminjb
committed
Add validation for pgbouncer logfile
This PR adds validation for v1 pgBouncer so that the logfile must either be in /tmp or one of the attached additional volumes. This PR also adds validation for v1beta1 that restricts the number of global config settings and requires a '.log' suffix on the logfile. (This may be reverted in the future as we are learning towards validation in v1 and documentation in v1beta1.) This PR also fixes an error in the OTEL setup around a custom logfile. Issues: [PGO-2565]
1 parent 01d1788 commit 937ee38

File tree

8 files changed

+286
-9
lines changed

8 files changed

+286
-9
lines changed

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

Lines changed: 16 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -14472,10 +14472,12 @@ spec:
1447214472
global:
1447314473
additionalProperties:
1447414474
type: string
14475-
description: |-
14476-
Settings that apply to the entire PgBouncer process.
14477-
More info: https://www.pgbouncer.org/config.html
14475+
maxProperties: 50
1447814476
type: object
14477+
x-kubernetes-map-type: granular
14478+
x-kubernetes-validations:
14479+
- message: logfile config must end with '.log'
14480+
rule: '!has(self.logfile) || self.logfile.endsWith(''.log'')'
1447914481
users:
1448014482
additionalProperties:
1448114483
type: string
@@ -16505,6 +16507,12 @@ spec:
1650516507
x-kubernetes-list-type: map
1650616508
type: object
1650716509
type: object
16510+
x-kubernetes-validations:
16511+
- message: config.global.logfile destination is restricted to
16512+
'/tmp/logs/pgbouncer/' or an existing additional volume
16513+
rule: self.?config.global.logfile.optMap(f, f.startsWith("/tmp/logs/pgbouncer/")
16514+
|| (self.?volumes.additional.hasValue() && self.volumes.additional.exists(v,
16515+
f.startsWith("/volumes/" + v.name)))).orValue(true)
1650816516
required:
1650916517
- pgBouncer
1651016518
type: object
@@ -33505,10 +33513,12 @@ spec:
3350533513
global:
3350633514
additionalProperties:
3350733515
type: string
33508-
description: |-
33509-
Settings that apply to the entire PgBouncer process.
33510-
More info: https://www.pgbouncer.org/config.html
33516+
maxProperties: 50
3351133517
type: object
33518+
x-kubernetes-map-type: granular
33519+
x-kubernetes-validations:
33520+
- message: logfile config must end with '.log'
33521+
rule: '!has(self.logfile) || self.logfile.endsWith(''.log'')'
3351233522
users:
3351333523
additionalProperties:
3351433524
type: string

‎internal/pgbouncer/reconcile.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -223,7 +223,7 @@ func Pod(
223223
if collector.OpenTelemetryLogsOrMetricsEnabled(ctx, inCluster) {
224224
collector.AddToPod(ctx, inCluster.Spec.Instrumentation, inCluster.Spec.ImagePullPolicy, inConfigMap,
225225
template, []corev1.VolumeMount{configVolumeMount}, string(inSecret.Data["pgbouncer-password"]),
226-
[]string{naming.PGBouncerLogPath}, true, true)
226+
[]string{logPath}, true, true)
227227
}
228228
}
229229

‎internal/pgbouncer/reconcile_test.go‎

Lines changed: 61 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -386,6 +386,12 @@ volumes:
386386
})
387387

388388
t.Run("WithOtelNoLogSet", func(t *testing.T) {
389+
featuresWithOtel := feature.NewGate()
390+
assert.NilError(t, featuresWithOtel.SetFromMap(map[string]bool{
391+
feature.OpenTelemetryLogs: true,
392+
feature.OpenTelemetryMetrics: true,
393+
}))
394+
ctx = feature.NewContext(context.Background(), featuresWithOtel)
389395
cluster.Spec.Instrumentation = &v1beta1.InstrumentationSpec{}
390396
logfile = "/tmp/pgbouncer.log"
391397

@@ -425,6 +431,59 @@ containers:
425431
- mountPath: /etc/pgbouncer
426432
name: pgbouncer-config
427433
readOnly: true
434+
- command:
435+
- bash
436+
- -ceu
437+
- --
438+
- "monitor() {\n\nmkdir -p '/tmp/receiver' && { chmod 0775 '/tmp/receiver' || :;
439+
}\nOTEL_PIDFILE=/tmp/otel.pid\n\nstart_otel_collector() {\n\techo \"Starting OTel
440+
Collector\"\n\t/otelcol-contrib --config /etc/otel-collector/config.yaml &\n\techo
441+
$! > $OTEL_PIDFILE\n}\nstart_otel_collector\n\nexec {fd}<> <(:||:)\nwhile read
442+
-r -t 5 -u \"${fd}\" ||:; do\n\tlogrotate -s /tmp/logrotate.status /etc/logrotate.d/logrotate.conf\n\tif
443+
[[ \"${directory}\" -nt \"/proc/self/fd/${fd}\" ]] && kill -HUP $(head -1 ${OTEL_PIDFILE?});\n\tthen\n\t\techo
444+
\"OTel configuration changed...\"\n\t\texec {fd}>&- && exec {fd}<> <(:||:)\n\t\tstat
445+
--format='Loaded configuration dated %y' \"${directory}\"\n\tfi\n\tif [[ ! -e
446+
/proc/$(head -1 ${OTEL_PIDFILE?}) ]] ; then\n\t\tstart_otel_collector\n\tfi\ndone\n};
447+
export directory=\"$1\"; export -f monitor; exec -a \"$0\" bash -ceu monitor"
448+
- collector
449+
- /etc/otel-collector
450+
env:
451+
- name: K8S_POD_NAMESPACE
452+
valueFrom:
453+
fieldRef:
454+
fieldPath: metadata.namespace
455+
- name: K8S_POD_NAME
456+
valueFrom:
457+
fieldRef:
458+
fieldPath: metadata.name
459+
- name: PGPASSWORD
460+
imagePullPolicy: Always
461+
name: collector
462+
ports:
463+
- containerPort: 9187
464+
name: otel-metrics
465+
protocol: TCP
466+
resources: {}
467+
securityContext:
468+
allowPrivilegeEscalation: false
469+
capabilities:
470+
drop:
471+
- ALL
472+
privileged: false
473+
readOnlyRootFilesystem: true
474+
runAsNonRoot: true
475+
seccompProfile:
476+
type: RuntimeDefault
477+
volumeMounts:
478+
- mountPath: /etc/pgbouncer
479+
name: pgbouncer-config
480+
readOnly: true
481+
- mountPath: /etc/otel-collector
482+
name: collector-config
483+
readOnly: true
484+
- mountPath: /etc/logrotate.d
485+
name: logrotate-config
486+
readOnly: true
428487
- command:
429488
- bash
430489
- -ceu
@@ -498,6 +557,8 @@ volumes:
498557
"logfile": "/volumes/required/mylog.log",
499558
}
500559
logfile = "/volumes/required/mylog.log"
560+
// Reset the instrumentation from the previous test
561+
cluster.Spec.Instrumentation = nil
501562

502563
call()
503564

‎internal/testing/validation/pgbouncer_test.go‎

Lines changed: 168 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,168 @@
1+
// Copyright 2021 - 2025 Crunchy Data Solutions, Inc.
2+
//
3+
// SPDX-License-Identifier: Apache-2.0
4+
5+
package validation
6+
7+
import (
8+
"context"
9+
"testing"
10+
11+
"gotest.tools/v3/assert"
12+
apierrors "k8s.io/apimachinery/pkg/api/errors"
13+
"sigs.k8s.io/controller-runtime/pkg/client"
14+
15+
"github.com/crunchydata/postgres-operator/internal/testing/require"
16+
v1 "github.com/crunchydata/postgres-operator/pkg/apis/postgres-operator.crunchydata.com/v1"
17+
)
18+
19+
func TestV1PGBouncerLogging(t *testing.T) {
20+
ctx := context.Background()
21+
cc := require.Kubernetes(t)
22+
t.Parallel()
23+
24+
namespace := require.Namespace(t, cc)
25+
26+
base := v1.NewPostgresCluster()
27+
base.Namespace = namespace.Name
28+
base.Name = "pgbouncer-logging"
29+
// required fields
30+
require.UnmarshalInto(t, &base.Spec, `{
31+
postgresVersion: 16,
32+
instances: [{
33+
dataVolumeClaimSpec: {
34+
accessModes: [ReadWriteOnce],
35+
resources: { requests: { storage: 1Mi } },
36+
},
37+
}],
38+
}`)
39+
40+
assert.NilError(t, cc.Create(ctx, base.DeepCopy(), client.DryRunAll),
41+
"expected this base to be valid")
42+
43+
t.Run("Can set logging on tmp with .log", func(t *testing.T) {
44+
tmp := base.DeepCopy()
45+
46+
require.UnmarshalInto(t, &tmp.Spec.Proxy, `{
47+
pgBouncer: {
48+
config: {
49+
global: {
50+
logfile: "/tmp/logs/pgbouncer/log.log"
51+
}
52+
}
53+
}
54+
}`)
55+
assert.NilError(t, cc.Create(ctx, base.DeepCopy(), client.DryRunAll),
56+
"expected this option to be valid")
57+
})
58+
59+
t.Run("Cannot set logging on tmp without .log", func(t *testing.T) {
60+
tmp := base.DeepCopy()
61+
62+
require.UnmarshalInto(t, &tmp.Spec.Proxy, `{
63+
pgBouncer: {
64+
config: {
65+
global: {
66+
logfile: "/tmp/logs/pgbouncer/log.txt"
67+
}
68+
}
69+
}
70+
}`)
71+
72+
err := cc.Create(ctx, tmp.DeepCopy(), client.DryRunAll)
73+
assert.Assert(t, apierrors.IsInvalid(err))
74+
assert.ErrorContains(t, err, "logfile config must end with '.log'")
75+
})
76+
77+
t.Run("Cannot set logging on tmp without correct subdir", func(t *testing.T) {
78+
tmp := base.DeepCopy()
79+
80+
require.UnmarshalInto(t, &tmp.Spec.Proxy, `{
81+
pgBouncer: {
82+
config: {
83+
global: {
84+
logfile: "/tmp/logs/log.log"
85+
}
86+
}
87+
}
88+
}`)
89+
90+
err := cc.Create(ctx, tmp.DeepCopy(), client.DryRunAll)
91+
assert.Assert(t, apierrors.IsInvalid(err))
92+
assert.ErrorContains(t, err, "logfile destination is restricted to '/tmp/logs/pgbouncer/' or an existing additional volume")
93+
94+
require.UnmarshalInto(t, &tmp.Spec.Proxy, `{
95+
pgBouncer: {
96+
config: {
97+
global: {
98+
logfile: "/tmp/pgbouncer/log.log"
99+
}
100+
}
101+
}
102+
}`)
103+
104+
err = cc.Create(ctx, tmp.DeepCopy(), client.DryRunAll)
105+
assert.Assert(t, apierrors.IsInvalid(err))
106+
assert.ErrorContains(t, err, "logfile destination is restricted to '/tmp/logs/pgbouncer/' or an existing additional volume")
107+
})
108+
109+
t.Run("Cannot set logging on volumes that don't exist", func(t *testing.T) {
110+
vol := base.DeepCopy()
111+
112+
require.UnmarshalInto(t, &vol.Spec.Proxy, `{
113+
pgBouncer: {
114+
config: {
115+
global: {
116+
logfile: "/volumes/logging/log.log"
117+
}
118+
}
119+
}
120+
}`)
121+
122+
err := cc.Create(ctx, vol.DeepCopy(), client.DryRunAll)
123+
assert.Assert(t, apierrors.IsInvalid(err))
124+
assert.ErrorContains(t, err, "logfile destination is restricted to '/tmp/logs/pgbouncer/' or an existing additional volume")
125+
})
126+
127+
t.Run("Cannot set logging elsewhere", func(t *testing.T) {
128+
vol := base.DeepCopy()
129+
130+
require.UnmarshalInto(t, &vol.Spec.Proxy, `{
131+
pgBouncer: {
132+
config: {
133+
global: {
134+
logfile: "/var/log.log"
135+
}
136+
}
137+
}
138+
}`)
139+
140+
err := cc.Create(ctx, vol.DeepCopy(), client.DryRunAll)
141+
assert.Assert(t, apierrors.IsInvalid(err))
142+
assert.ErrorContains(t, err, "logfile destination is restricted to '/tmp/logs/pgbouncer/' or an existing additional volume")
143+
})
144+
145+
t.Run("Can set logging on volumes that exist", func(t *testing.T) {
146+
vol := base.DeepCopy()
147+
148+
require.UnmarshalInto(t, &vol.Spec.Proxy, `{
149+
pgBouncer: {
150+
config: {
151+
global: {
152+
logfile: "/volumes/logging/log.log"
153+
}
154+
},
155+
volumes: {
156+
additional: [
157+
{
158+
name: logging,
159+
claimName: required-1
160+
}]
161+
}
162+
}
163+
}`)
164+
165+
assert.NilError(t, cc.Create(ctx, vol.DeepCopy(), client.DryRunAll),
166+
"expected this option to be valid")
167+
})
168+
}

‎pkg/apis/postgres-operator.crunchydata.com/v1/pgbouncer_types.go‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
// Copyright 2021 - 2025 Crunchy Data Solutions, Inc.
2+
//
3+
// SPDX-License-Identifier: Apache-2.0
4+
5+
package v1
6+
7+
import (
8+
"github.com/crunchydata/postgres-operator/pkg/apis/postgres-operator.crunchydata.com/v1beta1"
9+
)
10+
11+
// PGBouncerPodSpec defines the desired state of a PgBouncer connection pooler.
12+
// +kubebuilder:validation:XValidation:rule=`self.?config.global.logfile.optMap(f, f.startsWith("/tmp/logs/pgbouncer/") || (self.?volumes.additional.hasValue() && self.volumes.additional.exists(v, f.startsWith("/volumes/" + v.name)))).orValue(true)`,message=`config.global.logfile destination is restricted to '/tmp/logs/pgbouncer/' or an existing additional volume`
13+
type PGBouncerPodSpec struct {
14+
v1beta1.PGBouncerPodSpec `json:",inline"`
15+
}

‎pkg/apis/postgres-operator.crunchydata.com/v1/postgrescluster_types.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -602,7 +602,7 @@ type PostgresInstanceSetStatus struct {
602602
type PostgresProxySpec struct {
603603

604604
// Defines a PgBouncer proxy and connection pooler.
605-
PGBouncer *v1beta1.PGBouncerPodSpec `json:"pgBouncer"`
605+
PGBouncer *PGBouncerPodSpec `json:"pgBouncer"`
606606
}
607607

608608
// Default sets the defaults for any proxies that are set.

‎pkg/apis/postgres-operator.crunchydata.com/v1/zz_generated.deepcopy.go‎

Lines changed: 17 additions & 1 deletion
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

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

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,14 @@ type PGBouncerConfiguration struct {
2626

2727
// Settings that apply to the entire PgBouncer process.
2828
// More info: https://www.pgbouncer.org/config.html
29+
// ---
30+
// # Logging
31+
// +kubebuilder:validation:XValidation:rule=`!has(self.logfile) || self.logfile.endsWith('.log')`,message=`logfile config must end with '.log'`
32+
// +kubebuilder:validation:MaxProperties=50
33+
// See also XValidation rule on v1 PostgresProxySpec
34+
2935
// +optional
36+
// +mapType=granular
3037
Global map[string]string `json:"global,omitempty"`
3138

3239
// PgBouncer database definitions. The key is the database requested by a

0 commit comments

Comments
 (0)