Skip to content

Commit bb75c08

Browse files
cbandybenjaminjbdsessler7
committed
Validate the Postgres log_directory parameter in v1
This is a tightening of validation compared to v1beta1. The parameter must have one of a handful of prefixes. A spec-level rule ensures the value refers to a volume declared in the spec. Co-authored-by: Benjamin Blattberg <ben.blattberg@crunchydata.com> Co-authored-by: Drew Sessler <drew.sessler@crunchydata.com> Issue: PGO-2558
1 parent afa48a2 commit bb75c08

File tree

7 files changed

+361
-5
lines changed

7 files changed

+361
-5
lines changed

‎.github/workflows/test.yaml‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -53,7 +53,7 @@ jobs:
5353
strategy:
5454
fail-fast: false
5555
matrix:
56-
kubernetes: [v1.33, v1.28]
56+
kubernetes: [v1.30, v1.33]
5757
steps:
5858
- uses: actions/checkout@v5
5959
- uses: actions/setup-go@v6
@@ -87,7 +87,7 @@ jobs:
8787
strategy:
8888
fail-fast: false
8989
matrix:
90-
kubernetes: [v1.33, v1.28]
90+
kubernetes: [v1.30, v1.33]
9191
steps:
9292
- uses: actions/checkout@v5
9393
- uses: actions/setup-go@v6

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

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4939,6 +4939,13 @@ spec:
49394939
rule: '!has(self.logging_collector)'
49404940
- message: log_file_mode cannot be changed
49414941
rule: '!has(self.log_file_mode)'
4942+
- fieldPath: .log_directory
4943+
message: must start with "/pgdata/logs/postgres", "/pgtmp/logs/postgres",
4944+
"/pgwal/logs/postgres", "/volumes", or be "log" to keep logs
4945+
inside PGDATA
4946+
rule: self.?log_directory.optMap(v, type(v) == string && (v
4947+
== "log" || v.startsWith("/volumes") || ["/pgdata","/pgtmp","/pgwal","/volumes"].exists(p,
4948+
v == (p + "/logs/postgres") || v.startsWith(p + "/logs/postgres/")))).orValue(true)
49424949
type: object
49434950
customReplicationTLSSecret:
49444951
description: |-
@@ -18380,6 +18387,21 @@ spec:
1838018387
- instances
1838118388
- postgresVersion
1838218389
type: object
18390+
x-kubernetes-validations:
18391+
- fieldPath: .config.parameters.log_directory
18392+
message: all instances need "volumes.temp" to log in "/pgtmp"
18393+
rule: self.?config.parameters.log_directory.optMap(v, type(v) != string
18394+
|| !v.startsWith("/pgtmp/logs/postgres") || self.instances.all(i,
18395+
i.?volumes.temp.hasValue())).orValue(true)
18396+
- fieldPath: .config.parameters.log_directory
18397+
message: all instances need "walVolumeClaimSpec" to log in "/pgwal"
18398+
rule: self.?config.parameters.log_directory.optMap(v, type(v) != string
18399+
|| !v.startsWith("/pgwal/logs/postgres") || self.instances.all(i,
18400+
i.?walVolumeClaimSpec.hasValue())).orValue(true)
18401+
- fieldPath: .config.parameters.log_directory
18402+
message: all instances need an additional volume to log in "/volumes"
18403+
rule: self.?config.parameters.log_directory.optMap(v, type(v) != string
18404+
|| !v.startsWith("/volumes") || self.instances.all(i, i.?volumes.additional.hasValue())).orValue(true)
1838318405
status:
1838418406
description: PostgresClusterStatus defines the observed state of PostgresCluster
1838518407
properties:

‎internal/testing/cmp/cmp.go‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
package cmp
66

77
import (
8+
stdcmp "cmp"
89
"regexp"
910
"strings"
1011

@@ -51,6 +52,12 @@ func DeepEqual[T any](x, y T, opts ...gocmp.Option) Comparison {
5152
return gotest.DeepEqual(x, y, opts...)
5253
}
5354

55+
// Equal succeeds if x == y, the same as [gotest.tools/v3/assert.Equal].
56+
// The type constraint makes it easier to compare against numeric literals and typed constants.
57+
func Equal[T any](x, y T) Comparison {
58+
return gotest.Equal(x, y)
59+
}
60+
5461
// Len succeeds if actual has the expected length.
5562
func Len[Slice ~[]E, E any](actual Slice, expected int) Comparison {
5663
return gotest.Len(actual, expected)
@@ -85,6 +92,15 @@ func MarshalMatches(actual any, expected string) Comparison {
8592
))
8693
}
8794

95+
// Or is an alias to [stdcmp.Or] in the standard library:
96+
// - It returns the leftmost argument that is not zero.
97+
// - It returns zero when all its arguments are zero.
98+
//
99+
// This is here so test authors can import fewer "cmp" packages.
100+
func Or[T comparable](values ...T) T {
101+
return stdcmp.Or(values...)
102+
}
103+
88104
// Regexp succeeds if value contains any match of the regular expression.
89105
// The regular expression may be a *regexp.Regexp or a string that is a valid
90106
// regexp pattern.

‎internal/testing/validation/postgrescluster/postgres_config_test.go‎

Lines changed: 208 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,26 @@ func TestPostgresConfigParametersV1beta1(t *testing.T) {
5050
assert.Equal(t, u.GetAPIVersion(), "postgres-operator.crunchydata.com/v1beta1")
5151

5252
testPostgresConfigParametersCommon(t, cc, u)
53+
54+
t.Run("Logging", func(t *testing.T) {
55+
t.Run("Allowed", func(t *testing.T) {
56+
for _, tt := range []struct {
57+
key string
58+
value any
59+
}{
60+
{key: "log_directory", value: "anything"},
61+
} {
62+
t.Run(tt.key, func(t *testing.T) {
63+
cluster := u.DeepCopy()
64+
require.UnmarshalIntoField(t, cluster,
65+
require.Value(yaml.Marshal(tt.value)),
66+
"spec", "config", "parameters", tt.key)
67+
68+
assert.NilError(t, cc.Create(ctx, cluster, client.DryRunAll))
69+
})
70+
}
71+
})
72+
})
5373
}
5474

5575
func TestPostgresConfigParametersV1(t *testing.T) {
@@ -82,6 +102,194 @@ func TestPostgresConfigParametersV1(t *testing.T) {
82102
assert.Equal(t, u.GetAPIVersion(), "postgres-operator.crunchydata.com/v1")
83103

84104
testPostgresConfigParametersCommon(t, cc, u)
105+
106+
t.Run("Logging", func(t *testing.T) {
107+
t.Run("log_directory", func(t *testing.T) {
108+
volume := `{ accessModes: [ReadWriteOnce], resources: { requests: { storage: 1Mi } } }`
109+
110+
for _, tt := range []struct {
111+
name string
112+
value any
113+
valid bool
114+
message string
115+
instances string
116+
}{
117+
// Only a few prefixes are allowed.
118+
{valid: false, value: 99, message: `must start with "/`},
119+
{valid: false, value: "relative", message: `must start with "/`},
120+
{valid: false, value: "/absolute", message: `must start with "/pg`},
121+
122+
// These are the two acceptable directories inside /pgdata.
123+
{valid: true, value: "log"},
124+
{valid: true, value: "/pgdata/logs/postgres"},
125+
{valid: false, value: "/pgdata", message: `"/pgdata/logs/postgres"`},
126+
{valid: false, value: "/pgdata/elsewhere", message: `"/pgdata/logs/postgres"`},
127+
{valid: false, value: "/pgdata/sub/dir", message: `"/pgdata/logs/postgres"`},
128+
129+
// There is one acceptable directory inside /pgtmp, but every instance set needs a temp volume.
130+
{
131+
name: "two instance sets and two temp volumes",
132+
value: "/pgtmp/logs/postgres",
133+
instances: `[
134+
{ name: one, dataVolumeClaimSpec: ` + volume + `, volumes: { temp: ` + volume + ` } },
135+
{ name: two, dataVolumeClaimSpec: ` + volume + `, volumes: { temp: ` + volume + ` } },
136+
]`,
137+
valid: true,
138+
},
139+
{
140+
name: "two instance sets and one temp volume",
141+
value: "/pgtmp/logs/postgres",
142+
instances: `[
143+
{ name: one, dataVolumeClaimSpec: ` + volume + `, volumes: { temp: ` + volume + ` } },
144+
{ name: two, dataVolumeClaimSpec: ` + volume + ` },
145+
]`,
146+
valid: false,
147+
message: `all instances need "volumes.temp"`,
148+
},
149+
{
150+
name: "two instance sets and no temp volumes",
151+
value: "/pgtmp/logs/postgres",
152+
instances: `[
153+
{ name: one, dataVolumeClaimSpec: ` + volume + ` },
154+
{ name: two, dataVolumeClaimSpec: ` + volume + ` },
155+
]`,
156+
valid: false,
157+
message: `all instances need "volumes.temp"`,
158+
},
159+
160+
// These directories inside /pgtmp are unacceptable, regardless of volumes.
161+
{
162+
name: "no temp volumes",
163+
value: "/pgtmp/elsewhere",
164+
instances: `[{ name: any, dataVolumeClaimSpec: ` + volume + ` }]`,
165+
valid: false,
166+
message: `"/pgtmp/logs/postgres"`,
167+
},
168+
{
169+
name: "two temp volumes",
170+
value: "/pgtmp",
171+
instances: `[
172+
{ name: one, dataVolumeClaimSpec: ` + volume + `, volumes: { temp: ` + volume + ` } },
173+
{ name: two, dataVolumeClaimSpec: ` + volume + `, volumes: { temp: ` + volume + ` } },
174+
]`,
175+
valid: false,
176+
message: `"/pgtmp/logs/postgres"`,
177+
},
178+
179+
// There is one acceptable directory inside /pgwal, but every instance set needs a WAL volume.
180+
{
181+
name: "two instance sets and two WAL volumes",
182+
value: "/pgwal/logs/postgres",
183+
instances: `[
184+
{ name: one, dataVolumeClaimSpec: ` + volume + `, walVolumeClaimSpec: ` + volume + ` },
185+
{ name: two, dataVolumeClaimSpec: ` + volume + `, walVolumeClaimSpec: ` + volume + ` },
186+
]`,
187+
valid: true,
188+
},
189+
{
190+
name: "two instance sets and one WAL volume",
191+
value: "/pgwal/logs/postgres",
192+
instances: `[
193+
{ name: one, dataVolumeClaimSpec: ` + volume + `, walVolumeClaimSpec: ` + volume + ` },
194+
{ name: two, dataVolumeClaimSpec: ` + volume + ` },
195+
]`,
196+
valid: false,
197+
message: `all instances need "walVolumeClaimSpec"`,
198+
},
199+
{
200+
name: "two instance sets and no WAL volumes",
201+
value: "/pgwal/logs/postgres",
202+
instances: `[
203+
{ name: one, dataVolumeClaimSpec: ` + volume + ` },
204+
{ name: two, dataVolumeClaimSpec: ` + volume + ` },
205+
]`,
206+
valid: false,
207+
message: `all instances need "walVolumeClaimSpec"`,
208+
},
209+
210+
// These directories inside /pgwal are unacceptable, regardless of volumes.
211+
{
212+
name: "no WAL volumes",
213+
value: "/pgwal/elsewhere",
214+
instances: `[{ name: any, dataVolumeClaimSpec: ` + volume + ` }]`,
215+
valid: false,
216+
message: `"/pgwal/logs/postgres"`,
217+
},
218+
{
219+
name: "two WAL volumes",
220+
value: "/pgwal",
221+
instances: `[
222+
{ name: one, dataVolumeClaimSpec: ` + volume + `, walVolumeClaimSpec: ` + volume + ` },
223+
{ name: two, dataVolumeClaimSpec: ` + volume + `, walVolumeClaimSpec: ` + volume + ` },
224+
]`,
225+
valid: false,
226+
message: `"/pgwal/logs/postgres"`,
227+
},
228+
229+
// Directories inside /volumes are acceptable, but every instance set needs additional volumes.
230+
//
231+
// TODO(validation): This could be more precise and check the directory name of each additional
232+
// volume, but Kubernetes 1.33 incorrectly estimates the cost of volume.name:
233+
// https://github.com/kubernetes-sigs/controller-tools/pull/1270#issuecomment-3272211184
234+
{
235+
name: "two instance sets and two additional volumes",
236+
value: "/volumes/anything",
237+
instances: `[
238+
{ name: one, dataVolumeClaimSpec: ` + volume + `, volumes: { additional: [{ name: dir, claimName: a }] } },
239+
{ name: two, dataVolumeClaimSpec: ` + volume + `, volumes: { additional: [{ name: dir, claimName: b }] } },
240+
]`,
241+
valid: true,
242+
},
243+
{
244+
name: "two instance sets and one additional volume",
245+
value: "/volumes/anything",
246+
instances: `[
247+
{ name: one, dataVolumeClaimSpec: ` + volume + `, volumes: { additional: [{ name: dir, claimName: a }] } },
248+
{ name: two, dataVolumeClaimSpec: ` + volume + ` },
249+
]`,
250+
valid: false,
251+
message: `all instances need an additional volume`,
252+
},
253+
{
254+
name: "two instance sets and no additional volumes",
255+
value: "/volumes/anything",
256+
instances: `[
257+
{ name: one, dataVolumeClaimSpec: ` + volume + ` },
258+
{ name: two, dataVolumeClaimSpec: ` + volume + ` },
259+
]`,
260+
valid: false,
261+
message: `all instances need an additional volume`,
262+
},
263+
} {
264+
t.Run(cmp.Or(tt.name, fmt.Sprint(tt.valid, tt.value)), func(t *testing.T) {
265+
cluster := u.DeepCopy()
266+
if tt.instances != "" {
267+
require.UnmarshalIntoField(t, cluster, tt.instances, "spec", "instances")
268+
}
269+
require.UnmarshalIntoField(t, cluster, require.Value(yaml.Marshal(tt.value)),
270+
"spec", "config", "parameters", "log_directory")
271+
272+
err := cc.Create(ctx, cluster, client.DryRunAll)
273+
274+
if tt.valid {
275+
assert.NilError(t, err)
276+
assert.Equal(t, "", tt.message, "BUG IN TEST: no message expected when valid")
277+
} else {
278+
assert.Assert(t, apierrors.IsInvalid(err))
279+
280+
details := require.StatusErrorDetails(t, err)
281+
assert.Assert(t, cmp.Len(details.Causes, 1))
282+
283+
// https://issue.k8s.io/133761
284+
if details.Causes[0].Field != "spec.config.parameters.[log_directory]" {
285+
assert.Check(t, cmp.Equal(details.Causes[0].Field, "spec.config.parameters[log_directory]"))
286+
}
287+
assert.Assert(t, cmp.Contains(details.Causes[0].Message, tt.message))
288+
}
289+
})
290+
}
291+
})
292+
})
85293
}
86294

87295
func testPostgresConfigParametersCommon(t *testing.T, cc client.Client, base unstructured.Unstructured) {
@@ -155,7 +363,6 @@ func testPostgresConfigParametersCommon(t *testing.T, cc client.Client, base uns
155363
{valid: false, key: "logging_collector", value: "on", message: "unsafe"},
156364

157365
{valid: true, key: "log_destination", value: "anything"},
158-
{valid: true, key: "log_directory", value: "anything"},
159366
{valid: true, key: "log_filename", value: "anything"},
160367
{valid: true, key: "log_filename", value: "percent-%s-too"},
161368
{valid: true, key: "log_rotation_age", value: "7d"},

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

Lines changed: 71 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,71 @@
1+
// Copyright 2021 - 2025 Crunchy Data Solutions, Inc.
2+
//
3+
// SPDX-License-Identifier: Apache-2.0
4+
5+
package v1
6+
7+
import (
8+
corev1 "k8s.io/api/core/v1"
9+
"k8s.io/apimachinery/pkg/util/intstr"
10+
)
11+
12+
type PostgresConfigSpec struct {
13+
// Files to mount under "/etc/postgres".
14+
// ---
15+
// +optional
16+
Files []corev1.VolumeProjection `json:"files,omitempty"`
17+
18+
// Configuration parameters for the PostgreSQL server. Some values will
19+
// be reloaded without validation and some cause PostgreSQL to restart.
20+
// Some values cannot be changed at all.
21+
// More info: https://www.postgresql.org/docs/current/runtime-config.html
22+
// ---
23+
//
24+
// Postgres 17 has something like 350+ built-in parameters, but typically
25+
// an administrator will change only a handful of these.
26+
// +kubebuilder:validation:MaxProperties=50
27+
//
28+
// # File Locations
29+
// - https://www.postgresql.org/docs/current/runtime-config-file-locations.html
30+
//
31+
// +kubebuilder:validation:XValidation:rule=`!has(self.config_file) && !has(self.data_directory)`,message=`cannot change PGDATA path: config_file, data_directory`
32+
// +kubebuilder:validation:XValidation:rule=`!has(self.external_pid_file)`,message=`cannot change external_pid_file`
33+
// +kubebuilder:validation:XValidation:rule=`!has(self.hba_file) && !has(self.ident_file)`,message=`cannot change authentication path: hba_file, ident_file`
34+
//
35+
// # Connections
36+
// - https://www.postgresql.org/docs/current/runtime-config-connection.html
37+
//
38+
// +kubebuilder:validation:XValidation:rule=`!has(self.listen_addresses)`,message=`network connectivity is always enabled: listen_addresses`
39+
// +kubebuilder:validation:XValidation:rule=`!has(self.port)`,message=`change port using .spec.port instead`
40+
// +kubebuilder:validation:XValidation:rule=`!has(self.ssl) && !self.exists(k, k.startsWith("ssl_"))`,message=`TLS is always enabled`
41+
// +kubebuilder:validation:XValidation:rule=`!self.exists(k, k.startsWith("unix_socket_"))`,message=`domain socket paths cannot be changed`
42+
//
43+
// # Write Ahead Log
44+
// - https://www.postgresql.org/docs/current/runtime-config-wal.html
45+
//
46+
// +kubebuilder:validation:XValidation:rule=`!has(self.wal_level) || self.wal_level in ["logical"]`,message=`wal_level must be "replica" or higher`
47+
// +kubebuilder:validation:XValidation:rule=`!has(self.wal_log_hints)`,message=`wal_log_hints are always enabled`
48+
// +kubebuilder:validation:XValidation:rule=`!has(self.archive_mode) && !has(self.archive_command) && !has(self.restore_command)`
49+
// +kubebuilder:validation:XValidation:rule=`!has(self.recovery_target) && !self.exists(k, k.startsWith("recovery_target_"))`
50+
//
51+
// # Replication
52+
// - https://www.postgresql.org/docs/current/runtime-config-replication.html
53+
//
54+
// +kubebuilder:validation:XValidation:rule=`!has(self.hot_standby)`,message=`hot_standby is always enabled`
55+
// +kubebuilder:validation:XValidation:rule=`!has(self.synchronous_standby_names)`
56+
// +kubebuilder:validation:XValidation:rule=`!has(self.primary_conninfo) && !has(self.primary_slot_name)`
57+
// +kubebuilder:validation:XValidation:rule=`!has(self.recovery_min_apply_delay)`,message=`delayed replication is not supported at this time`
58+
//
59+
// # Logging
60+
// - https://www.postgresql.org/docs/current/runtime-config-logging.html
61+
//
62+
// +kubebuilder:validation:XValidation:rule=`!has(self.cluster_name)`,message=`cluster_name is derived from the PostgresCluster name`
63+
// +kubebuilder:validation:XValidation:rule=`!has(self.logging_collector)`,message=`disabling logging_collector is unsafe`
64+
// +kubebuilder:validation:XValidation:rule=`!has(self.log_file_mode)`,message=`log_file_mode cannot be changed`
65+
//
66+
// +kubebuilder:validation:XValidation:fieldPath=`.log_directory`,message=`must start with "/pgdata/logs/postgres", "/pgtmp/logs/postgres", "/pgwal/logs/postgres", "/volumes", or be "log" to keep logs inside PGDATA`,rule=`self.?log_directory.optMap(v, type(v) == string && (v == "log" || v.startsWith("/volumes") || ["/pgdata","/pgtmp","/pgwal","/volumes"].exists(p, v == (p + "/logs/postgres") || v.startsWith(p + "/logs/postgres/")))).orValue(true)`
67+
//
68+
// +mapType=granular
69+
// +optional
70+
Parameters map[string]intstr.IntOrString `json:"parameters,omitempty"`
71+
}

0 commit comments

Comments
 (0)