Skip to content

Commit edb797c

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 <[email protected]> Co-authored-by: Drew Sessler <[email protected]> Issue: PGO-2558
1 parent 406e069 commit edb797c

File tree

7 files changed

+634
-5
lines changed

7 files changed

+634
-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
@@ -4996,6 +4996,13 @@ spec:
49964996
rule: '!has(self.logging_collector)'
49974997
- message: log_file_mode cannot be changed
49984998
rule: '!has(self.log_file_mode)'
4999+
- fieldPath: .log_directory
5000+
message: must start with "/pgdata/logs/postgres", "/pgtmp/logs/postgres",
5001+
"/pgwal/logs/postgres", "/volumes", or be "log" to keep logs
5002+
inside PGDATA
5003+
rule: self.?log_directory.optMap(v, type(v) == string && (v
5004+
== "log" || v.startsWith("/volumes") || ["/pgdata","/pgtmp","/pgwal","/volumes"].exists(p,
5005+
v == (p + "/logs/postgres") || v.startsWith(p + "/logs/postgres/")))).orValue(true)
49995006
type: object
50005007
customReplicationTLSSecret:
50015008
description: |-
@@ -18649,6 +18656,21 @@ spec:
1864918656
- instances
1865018657
- postgresVersion
1865118658
type: object
18659+
x-kubernetes-validations:
18660+
- fieldPath: .config.parameters.log_directory
18661+
message: all instances need "volumes.temp" to log in "/pgtmp"
18662+
rule: self.?config.parameters.log_directory.optMap(v, type(v) != string
18663+
|| !v.startsWith("/pgtmp/logs/postgres") || self.instances.all(i,
18664+
i.?volumes.temp.hasValue())).orValue(true)
18665+
- fieldPath: .config.parameters.log_directory
18666+
message: all instances need "walVolumeClaimSpec" to log in "/pgwal"
18667+
rule: self.?config.parameters.log_directory.optMap(v, type(v) != string
18668+
|| !v.startsWith("/pgwal/logs/postgres") || self.instances.all(i,
18669+
i.?walVolumeClaimSpec.hasValue())).orValue(true)
18670+
- fieldPath: .config.parameters.log_directory
18671+
message: all instances need an additional volume to log in "/volumes"
18672+
rule: self.?config.parameters.log_directory.optMap(v, type(v) != string
18673+
|| !v.startsWith("/volumes") || self.instances.all(i, i.?volumes.additional.hasValue())).orValue(true)
1865218674
status:
1865318675
description: PostgresClusterStatus defines the observed state of PostgresCluster
1865418676
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 assertion 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"},

0 commit comments

Comments
 (0)