Skip to content

Commit 743dc39

Browse files
benjaminjbbenjaminjb
authored
Add validation for pgbouncer logfile (#4280)
* 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 PR also fixes an error in the OTEL setup around a custom logfile. Issues: [PGO-2565]
1 parent 2ffcabc commit 743dc39

File tree

9 files changed

+312
-3
lines changed

9 files changed

+312
-3
lines changed

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

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14284,7 +14284,12 @@ spec:
1428414284
description: |-
1428514285
Settings that apply to the entire PgBouncer process.
1428614286
More info: https://www.pgbouncer.org/config.html
14287+
maxProperties: 50
1428714288
type: object
14289+
x-kubernetes-map-type: granular
14290+
x-kubernetes-validations:
14291+
- message: logfile config must end with '.log'
14292+
rule: '!has(self.logfile) || self.logfile.endsWith(''.log'')'
1428814293
users:
1428914294
additionalProperties:
1429014295
type: string
@@ -16258,6 +16263,12 @@ spec:
1625816263
x-kubernetes-list-type: map
1625916264
type: object
1626016265
type: object
16266+
x-kubernetes-validations:
16267+
- message: config.global.logfile destination is restricted to
16268+
'/tmp/logs/pgbouncer/' or an existing additional volume
16269+
rule: self.?config.global.logfile.optMap(f, f.startsWith("/tmp/logs/pgbouncer/")
16270+
|| (self.?volumes.additional.hasValue() && self.volumes.additional.exists(v,
16271+
f.startsWith("/volumes/" + v.name)))).orValue(true)
1626116272
required:
1626216273
- pgBouncer
1626316274
type: object
@@ -33063,7 +33074,12 @@ spec:
3306333074
description: |-
3306433075
Settings that apply to the entire PgBouncer process.
3306533076
More info: https://www.pgbouncer.org/config.html
33077+
maxProperties: 50
3306633078
type: object
33079+
x-kubernetes-map-type: granular
33080+
x-kubernetes-validations:
33081+
- message: logfile config must end with '.log'
33082+
rule: '!has(self.logfile) || self.logfile.endsWith(''.log'')'
3306733083
users:
3306833084
additionalProperties:
3306933085
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
@@ -613,7 +613,7 @@ type PostgresInstanceSetStatus struct {
613613
type PostgresProxySpec struct {
614614

615615
// Defines a PgBouncer proxy and connection pooler.
616-
PGBouncer *v1beta1.PGBouncerPodSpec `json:"pgBouncer"`
616+
PGBouncer *PGBouncerPodSpec `json:"pgBouncer"`
617617
}
618618

619619
// 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)