Skip to content

Commit c95679a

Browse files
cbandycbandy
committed
Share PostgresCluster validation tests across API versions
Tests are shared by manipulating unstructured values rather structs. This exposed a bug in HBA LDAP rule validation that was masked by "omitempty" and "omitzero" tags on struct fields.
1 parent 087c08d commit c95679a

File tree

8 files changed

+749
-506
lines changed

8 files changed

+749
-506
lines changed

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

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -136,8 +136,8 @@ spec:
136136
"ldapsearchattribute", or "ldapsearchfilter" options with
137137
"ldapprefix" or "ldapsuffix" options
138138
rule: has(self.hba) || self.method != "ldap" || !has(self.options)
139-
|| [["ldapprefix","ldapsuffix"], ["ldapbasedn","ldapbinddn","ldapbindpasswd","ldapsearchattribute","ldapsearchfilter"]].exists_one(a,
140-
a.exists(k, k in self.options))
139+
|| 2 > size([["ldapprefix","ldapsuffix"], ["ldapbasedn","ldapbinddn","ldapbindpasswd","ldapsearchattribute","ldapsearchfilter"]].filter(a,
140+
a.exists(k, k in self.options)))
141141
- message: the "radius" method requires "radiusservers" and
142142
"radiussecrets" options
143143
rule: has(self.hba) || self.method != "radius" || (has(self.options)
@@ -18767,8 +18767,8 @@ spec:
1876718767
"ldapsearchattribute", or "ldapsearchfilter" options with
1876818768
"ldapprefix" or "ldapsuffix" options
1876918769
rule: has(self.hba) || self.method != "ldap" || !has(self.options)
18770-
|| [["ldapprefix","ldapsuffix"], ["ldapbasedn","ldapbinddn","ldapbindpasswd","ldapsearchattribute","ldapsearchfilter"]].exists_one(a,
18771-
a.exists(k, k in self.options))
18770+
|| 2 > size([["ldapprefix","ldapsuffix"], ["ldapbasedn","ldapbinddn","ldapbindpasswd","ldapsearchattribute","ldapsearchfilter"]].filter(a,
18771+
a.exists(k, k in self.options)))
1877218772
- message: the "radius" method requires "radiusservers" and
1877318773
"radiussecrets" options
1877418774
rule: has(self.hba) || self.method != "radius" || (has(self.options)

‎internal/testing/require/encoding.go

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ import (
99
"testing"
1010

1111
"gotest.tools/v3/assert"
12+
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
1213
"sigs.k8s.io/json"
1314
"sigs.k8s.io/yaml"
1415
)
@@ -37,3 +38,24 @@ func UnmarshalInto[Data ~string | ~[]byte, Destination *T, T any](
3738
assert.NilError(t, err)
3839
assert.NilError(t, errors.Join(strict...))
3940
}
41+
42+
// UnmarshalIntoField parses input as YAML (or JSON) the same way as the Kubernetes API Server.
43+
// The result goes into a (nested) field of output. It calls t.Fatal when something fails.
44+
func UnmarshalIntoField[Data ~string | ~[]byte](
45+
t testing.TB, output *unstructured.Unstructured, input Data, fields ...string,
46+
) {
47+
t.Helper()
48+
49+
if len(fields) == 0 {
50+
t.Fatal("BUG: called without a destination")
51+
}
52+
53+
if output.Object == nil {
54+
output.Object = map[string]any{}
55+
}
56+
57+
var value any
58+
UnmarshalInto(t, &value, []byte(input))
59+
60+
assert.NilError(t, unstructured.SetNestedField(output.Object, value, fields...))
61+
}

‎internal/testing/require/encoding_test.go

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,10 +8,14 @@ import (
88
"reflect"
99
"testing"
1010

11+
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
12+
1113
"github.com/crunchydata/postgres-operator/internal/testing/require"
1214
)
1315

1416
func TestUnmarshalInto(t *testing.T) {
17+
t.Parallel()
18+
1519
for _, tt := range []struct {
1620
input string
1721
expected any
@@ -39,3 +43,41 @@ func TestUnmarshalInto(t *testing.T) {
3943
}
4044
}
4145
}
46+
47+
func TestUnmarshalIntoField(t *testing.T) {
48+
t.Parallel()
49+
50+
var u unstructured.Unstructured
51+
52+
t.Run("NestedString", func(t *testing.T) {
53+
u.Object = nil
54+
require.UnmarshalIntoField(t, &u, `asdf`, "spec", "nested", "field")
55+
56+
if !reflect.DeepEqual(u.Object, map[string]any{
57+
"spec": map[string]any{
58+
"nested": map[string]any{
59+
"field": "asdf",
60+
},
61+
},
62+
}) {
63+
t.Fatalf("got %[1]T(%#[1]v)", u.Object)
64+
}
65+
})
66+
67+
t.Run("Numeric", func(t *testing.T) {
68+
u.Object = nil
69+
require.UnmarshalIntoField(t, &u, `99`, "one")
70+
require.UnmarshalIntoField(t, &u, `5.7`, "two")
71+
72+
// Kubernetes distinguishes between integral and fractional numbers.
73+
if !reflect.DeepEqual(u.Object, map[string]any{
74+
"one": int64(99),
75+
"two": float64(5.7),
76+
}) {
77+
t.Fatalf("got %[1]T(%#[1]v)", u.Object)
78+
}
79+
})
80+
81+
// Correctly fails with: BUG: called without a destination
82+
// require.UnmarshalIntoField(t, &u, `true`)
83+
}

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

Lines changed: 272 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,272 @@
1+
// Copyright 2021 - 2025 Crunchy Data Solutions, Inc.
2+
//
3+
// SPDX-License-Identifier: Apache-2.0
4+
5+
package validation
6+
7+
import (
8+
"fmt"
9+
"testing"
10+
11+
"gotest.tools/v3/assert"
12+
apierrors "k8s.io/apimachinery/pkg/api/errors"
13+
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
14+
"sigs.k8s.io/controller-runtime/pkg/client"
15+
"sigs.k8s.io/yaml"
16+
17+
"github.com/crunchydata/postgres-operator/internal/testing/cmp"
18+
"github.com/crunchydata/postgres-operator/internal/testing/require"
19+
v1 "github.com/crunchydata/postgres-operator/pkg/apis/postgres-operator.crunchydata.com/v1"
20+
"github.com/crunchydata/postgres-operator/pkg/apis/postgres-operator.crunchydata.com/v1beta1"
21+
)
22+
23+
func TestPostgresAuthenticationV1beta1(t *testing.T) {
24+
ctx := t.Context()
25+
cc := require.Kubernetes(t)
26+
t.Parallel()
27+
28+
namespace := require.Namespace(t, cc)
29+
base := v1beta1.NewPostgresCluster()
30+
31+
// required fields
32+
require.UnmarshalInto(t, &base.Spec, `{
33+
postgresVersion: 16,
34+
instances: [{
35+
dataVolumeClaimSpec: {
36+
accessModes: [ReadWriteOnce],
37+
resources: { requests: { storage: 1Mi } },
38+
},
39+
}],
40+
}`)
41+
42+
base.Namespace = namespace.Name
43+
base.Name = "postgres-authentication-rules"
44+
45+
assert.NilError(t, cc.Create(ctx, base.DeepCopy(), client.DryRunAll),
46+
"expected this base cluster to be valid")
47+
48+
var u unstructured.Unstructured
49+
require.UnmarshalInto(t, &u, require.Value(yaml.Marshal(base)))
50+
assert.Equal(t, u.GetAPIVersion(), "postgres-operator.crunchydata.com/v1beta1")
51+
52+
testPostgresAuthenticationCommon(t, cc, u)
53+
}
54+
55+
func TestPostgresAuthenticationV1(t *testing.T) {
56+
ctx := t.Context()
57+
cc := require.KubernetesAtLeast(t, "1.30")
58+
t.Parallel()
59+
60+
namespace := require.Namespace(t, cc)
61+
base := v1.NewPostgresCluster()
62+
63+
// required fields
64+
require.UnmarshalInto(t, &base.Spec, `{
65+
postgresVersion: 16,
66+
instances: [{
67+
dataVolumeClaimSpec: {
68+
accessModes: [ReadWriteOnce],
69+
resources: { requests: { storage: 1Mi } },
70+
},
71+
}],
72+
}`)
73+
74+
base.Namespace = namespace.Name
75+
base.Name = "postgres-authentication-rules"
76+
77+
assert.NilError(t, cc.Create(ctx, base.DeepCopy(), client.DryRunAll),
78+
"expected this base cluster to be valid")
79+
80+
var u unstructured.Unstructured
81+
require.UnmarshalInto(t, &u, require.Value(yaml.Marshal(base)))
82+
assert.Equal(t, u.GetAPIVersion(), "postgres-operator.crunchydata.com/v1")
83+
84+
testPostgresAuthenticationCommon(t, cc, u)
85+
}
86+
87+
func testPostgresAuthenticationCommon(t *testing.T, cc client.Client, base unstructured.Unstructured) {
88+
ctx := t.Context()
89+
90+
t.Run("OneTopLevel", func(t *testing.T) {
91+
cluster := base.DeepCopy()
92+
require.UnmarshalIntoField(t, cluster, `{
93+
rules: [
94+
{ connection: host, hba: anything },
95+
{ users: [alice, bob], hba: anything },
96+
],
97+
}`, "spec", "authentication")
98+
99+
err := cc.Create(ctx, cluster, client.DryRunAll)
100+
assert.Assert(t, apierrors.IsInvalid(err))
101+
102+
status := require.StatusError(t, err)
103+
assert.Assert(t, status.Details != nil)
104+
assert.Assert(t, cmp.Len(status.Details.Causes, 2))
105+
106+
for i, cause := range status.Details.Causes {
107+
assert.Equal(t, cause.Field, fmt.Sprintf("spec.authentication.rules[%d]", i))
108+
assert.Assert(t, cmp.Contains(cause.Message, "cannot be combined"))
109+
}
110+
})
111+
112+
t.Run("NoInclude", func(t *testing.T) {
113+
cluster := base.DeepCopy()
114+
require.UnmarshalIntoField(t, cluster, `{
115+
rules: [
116+
{ hba: 'include "/etc/passwd"' },
117+
{ hba: ' include_dir /tmp' },
118+
{ hba: 'include_if_exists postgresql.auto.conf' },
119+
],
120+
}`, "spec", "authentication")
121+
122+
err := cc.Create(ctx, cluster, client.DryRunAll)
123+
assert.Assert(t, apierrors.IsInvalid(err))
124+
125+
status := require.StatusError(t, err)
126+
assert.Assert(t, status.Details != nil)
127+
assert.Assert(t, cmp.Len(status.Details.Causes, 3))
128+
129+
for i, cause := range status.Details.Causes {
130+
assert.Equal(t, cause.Field, fmt.Sprintf("spec.authentication.rules[%d].hba", i))
131+
assert.Assert(t, cmp.Contains(cause.Message, "cannot include"))
132+
}
133+
})
134+
135+
t.Run("NoStructuredTrust", func(t *testing.T) {
136+
cluster := base.DeepCopy()
137+
require.UnmarshalIntoField(t, cluster, `{
138+
rules: [
139+
{ connection: local, method: trust },
140+
{ connection: hostssl, method: trust },
141+
{ connection: hostgssenc, method: trust },
142+
],
143+
}`, "spec", "authentication")
144+
145+
err := cc.Create(ctx, cluster, client.DryRunAll)
146+
assert.Assert(t, apierrors.IsInvalid(err))
147+
148+
status := require.StatusError(t, err)
149+
assert.Assert(t, status.Details != nil)
150+
assert.Assert(t, cmp.Len(status.Details.Causes, 3))
151+
152+
for i, cause := range status.Details.Causes {
153+
assert.Equal(t, cause.Field, fmt.Sprintf("spec.authentication.rules[%d].method", i))
154+
assert.Assert(t, cmp.Contains(cause.Message, "unsafe"))
155+
}
156+
})
157+
158+
t.Run("LDAP", func(t *testing.T) {
159+
t.Run("Required", func(t *testing.T) {
160+
cluster := base.DeepCopy()
161+
require.UnmarshalIntoField(t, cluster, `{
162+
rules: [
163+
{ connection: hostssl, method: ldap },
164+
{ connection: hostssl, method: ldap, options: {} },
165+
{ connection: hostssl, method: ldap, options: { ldapbinddn: any } },
166+
],
167+
}`, "spec", "authentication")
168+
169+
err := cc.Create(ctx, cluster, client.DryRunAll)
170+
assert.Assert(t, apierrors.IsInvalid(err))
171+
172+
status := require.StatusError(t, err)
173+
assert.Assert(t, status.Details != nil)
174+
assert.Assert(t, cmp.Len(status.Details.Causes, 3))
175+
176+
for i, cause := range status.Details.Causes {
177+
assert.Equal(t, cause.Field, fmt.Sprintf("spec.authentication.rules[%d]", i), "%#v", cause)
178+
assert.Assert(t, cmp.Contains(cause.Message, `"ldap" method requires`))
179+
}
180+
181+
// These are valid.
182+
183+
unstructured.RemoveNestedField(cluster.Object, "spec", "authentication")
184+
require.UnmarshalIntoField(t, cluster, `{
185+
rules: [
186+
{ connection: hostssl, method: ldap, options: { ldapbasedn: any } },
187+
{ connection: hostssl, method: ldap, options: { ldapprefix: any } },
188+
{ connection: hostssl, method: ldap, options: { ldapsuffix: any } },
189+
],
190+
}`, "spec", "authentication")
191+
assert.NilError(t, cc.Create(ctx, cluster, client.DryRunAll))
192+
})
193+
194+
t.Run("Mixed", func(t *testing.T) {
195+
// Some options cannot be combined with others.
196+
197+
cluster := base.DeepCopy()
198+
require.UnmarshalIntoField(t, cluster, `{
199+
rules: [
200+
{ connection: hostssl, method: ldap, options: { ldapbinddn: any, ldapprefix: other } },
201+
{ connection: hostssl, method: ldap, options: { ldapbasedn: any, ldapsuffix: other } },
202+
],
203+
}`, "spec", "authentication")
204+
205+
err := cc.Create(ctx, cluster, client.DryRunAll)
206+
assert.Assert(t, apierrors.IsInvalid(err))
207+
208+
status := require.StatusError(t, err)
209+
assert.Assert(t, status.Details != nil)
210+
assert.Assert(t, cmp.Len(status.Details.Causes, 2))
211+
212+
for i, cause := range status.Details.Causes {
213+
assert.Equal(t, cause.Field, fmt.Sprintf("spec.authentication.rules[%d]", i), "%#v", cause)
214+
assert.Assert(t, cmp.Regexp(`cannot use .+? options with .+? options`, cause.Message))
215+
}
216+
217+
// These combinations are allowed.
218+
219+
unstructured.RemoveNestedField(cluster.Object, "spec", "authentication")
220+
require.UnmarshalIntoField(t, cluster, `{
221+
rules: [
222+
{ connection: hostssl, method: ldap, options: { ldapprefix: one, ldapsuffix: two } },
223+
{ connection: hostssl, method: ldap, options: { ldapbasedn: one, ldapbinddn: two } },
224+
{ connection: hostssl, method: ldap, options: {
225+
ldapbasedn: one, ldapsearchattribute: two, ldapsearchfilter: three,
226+
} },
227+
],
228+
}`, "spec", "authentication")
229+
assert.NilError(t, cc.Create(ctx, cluster, client.DryRunAll))
230+
})
231+
})
232+
233+
t.Run("RADIUS", func(t *testing.T) {
234+
t.Run("Required", func(t *testing.T) {
235+
cluster := base.DeepCopy()
236+
require.UnmarshalIntoField(t, cluster, `{
237+
rules: [
238+
{ connection: hostssl, method: radius },
239+
{ connection: hostssl, method: radius, options: {} },
240+
{ connection: hostssl, method: radius, options: { radiusidentifiers: any } },
241+
{ connection: hostssl, method: radius, options: { radiusservers: any } },
242+
{ connection: hostssl, method: radius, options: { radiussecrets: any } },
243+
],
244+
}`, "spec", "authentication")
245+
246+
err := cc.Create(ctx, cluster, client.DryRunAll)
247+
assert.Assert(t, apierrors.IsInvalid(err))
248+
249+
status := require.StatusError(t, err)
250+
assert.Assert(t, status.Details != nil)
251+
assert.Assert(t, cmp.Len(status.Details.Causes, 5))
252+
253+
for i, cause := range status.Details.Causes {
254+
assert.Equal(t, cause.Field, fmt.Sprintf("spec.authentication.rules[%d]", i), "%#v", cause)
255+
assert.Assert(t, cmp.Contains(cause.Message, `"radius" method requires`))
256+
}
257+
258+
// These are valid.
259+
260+
unstructured.RemoveNestedField(cluster.Object, "spec", "authentication")
261+
require.UnmarshalIntoField(t, cluster, `{
262+
rules: [
263+
{ connection: hostssl, method: radius, options: { radiusservers: one, radiussecrets: two } },
264+
{ connection: hostssl, method: radius, options: {
265+
radiusservers: one, radiussecrets: two, radiusports: three,
266+
} },
267+
],
268+
}`, "spec", "authentication")
269+
assert.NilError(t, cc.Create(ctx, cluster, client.DryRunAll))
270+
})
271+
})
272+
}

0 commit comments

Comments
 (0)