Skip to content

Commit daa6b46

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 daa6b46

File tree

8 files changed

+298
-9
lines changed

8 files changed

+298
-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: 73 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -142,6 +142,10 @@ func TestPod(t *testing.T) {
142142
t.Parallel()
143143

144144
features := feature.NewGate()
145+
assert.NilError(t, features.SetFromMap(map[string]bool{
146+
feature.OpenTelemetryLogs: true,
147+
feature.OpenTelemetryMetrics: true,
148+
}))
145149
ctx := feature.NewContext(context.Background(), features)
146150

147151
cluster := new(v1beta1.PostgresCluster)
@@ -463,6 +467,59 @@ containers:
463467
- mountPath: /etc/pgbouncer
464468
name: pgbouncer-config
465469
readOnly: true
470+
- command:
471+
- bash
472+
- -ceu
473+
- --
474+
- "monitor() {\n\nmkdir -p '/tmp/receiver' && { chmod 0775 '/tmp/receiver' || :;
475+
}\nOTEL_PIDFILE=/tmp/otel.pid\n\nstart_otel_collector() {\n\techo \"Starting OTel
476+
Collector\"\n\t/otelcol-contrib --config /etc/otel-collector/config.yaml &\n\techo
477+
$! > $OTEL_PIDFILE\n}\nstart_otel_collector\n\nexec {fd}<> <(:||:)\nwhile read
478+
-r -t 5 -u \"${fd}\" ||:; do\n\tlogrotate -s /tmp/logrotate.status /etc/logrotate.d/logrotate.conf\n\tif
479+
[[ \"${directory}\" -nt \"/proc/self/fd/${fd}\" ]] && kill -HUP $(head -1 ${OTEL_PIDFILE?});\n\tthen\n\t\techo
480+
\"OTel configuration changed...\"\n\t\texec {fd}>&- && exec {fd}<> <(:||:)\n\t\tstat
481+
--format='Loaded configuration dated %y' \"${directory}\"\n\tfi\n\tif [[ ! -e
482+
/proc/$(head -1 ${OTEL_PIDFILE?}) ]] ; then\n\t\tstart_otel_collector\n\tfi\ndone\n};
483+
export directory=\"$1\"; export -f monitor; exec -a \"$0\" bash -ceu monitor"
484+
- collector
485+
- /etc/otel-collector
486+
env:
487+
- name: K8S_POD_NAMESPACE
488+
valueFrom:
489+
fieldRef:
490+
fieldPath: metadata.namespace
491+
- name: K8S_POD_NAME
492+
valueFrom:
493+
fieldRef:
494+
fieldPath: metadata.name
495+
- name: PGPASSWORD
496+
imagePullPolicy: Always
497+
name: collector
498+
ports:
499+
- containerPort: 9187
500+
name: otel-metrics
501+
protocol: TCP
502+
resources: {}
503+
securityContext:
504+
allowPrivilegeEscalation: false
505+
capabilities:
506+
drop:
507+
- ALL
508+
privileged: false
509+
readOnlyRootFilesystem: true
510+
runAsNonRoot: true
511+
seccompProfile:
512+
type: RuntimeDefault
513+
volumeMounts:
514+
- mountPath: /etc/pgbouncer
515+
name: pgbouncer-config
516+
readOnly: true
517+
- mountPath: /etc/otel-collector
518+
name: collector-config
519+
readOnly: true
520+
- mountPath: /etc/logrotate.d
521+
name: logrotate-config
522+
readOnly: true
466523
volumes:
467524
- name: pgbouncer-config
468525
projected:
@@ -490,6 +547,20 @@ volumes:
490547
items:
491548
- key: ca.crt
492549
path: ~postgres-operator/backend-ca.crt
550+
- name: logrotate-config
551+
projected:
552+
sources:
553+
- configMap:
554+
items:
555+
- key: logrotate.conf
556+
path: logrotate.conf
557+
- name: collector-config
558+
projected:
559+
sources:
560+
- configMap:
561+
items:
562+
- key: collector.yaml
563+
path: config.yaml
493564
`))
494565
})
495566

@@ -498,6 +569,8 @@ volumes:
498569
"logfile": "/volumes/required/mylog.log",
499570
}
500571
logfile = "/volumes/required/mylog.log"
572+
// Reset the instrumentation from the previous test
573+
cluster.Spec.Instrumentation = nil
501574

502575
call()
503576

‎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)