Share PostgresCluster validation tests across API versions #4220
Conversation
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.
| { connection: hostssl, method: ldap }, | ||
| { connection: hostssl, method: ldap, options: {} }, | ||
| { connection: hostssl, method: ldap, options: { ldapbinddn: any } }, |
There was a problem hiding this comment.
The options: {} case is what passed with v1beta1 and v1 structs but failed with unstructured. I suspect the empty object was silently being omitted, causing this case to match the one above it.
| @@ -136,8 +136,8 @@ spec: | |||
| "ldapsearchattribute", or "ldapsearchfilter" options with | |||
| "ldapprefix" or "ldapsuffix" options | |||
| rule: has(self.hba) || self.method != "ldap" || !has(self.options) | |||
| || [["ldapprefix","ldapsuffix"], ["ldapbasedn","ldapbinddn","ldapbindpasswd","ldapsearchattribute","ldapsearchfilter"]].exists_one(a, | |||
| a.exists(k, k in self.options)) | |||
| || 2 > size([["ldapprefix","ldapsuffix"], ["ldapbasedn","ldapbinddn","ldapbindpasswd","ldapsearchattribute","ldapsearchfilter"]].filter(a, | |||
There was a problem hiding this comment.
The !has(self.option) portion was being tested twice when using v1beta1 and v1 structs.
The unstructured value emitted options: {} which showed that exists_one is the wrong behavior:
- With
options: {}the field "has options" and has zero of ldapprefix, ldapbasedn, etc. - The correct behavior is "has options" and "at most one" of ldapprefix, ldapbasedn, etc.
| var u unstructured.Unstructured | ||
| require.UnmarshalInto(t, &u, require.Value(yaml.Marshal(base))) | ||
| assert.Equal(t, u.GetAPIVersion(), "postgres-operator.crunchydata.com/v1beta1") | ||
|
|
||
| testPostgresAuthenticationCommon(t, cc, u) |
There was a problem hiding this comment.
This is the pattern I used to share tests between v1beta1 and v1. Notice L82-84 of this same file.
|
|
||
| func TestPostgresAuthenticationV1(t *testing.T) { | ||
| ctx := t.Context() | ||
| cc := require.KubernetesAtLeast(t, "1.30") |
There was a problem hiding this comment.
These tests pass with v1 on K8s before 1.30, but the plan is for v1 to be 1.30+.
| }, | ||
| }, | ||
| }) { | ||
| t.Fatalf("got %[1]T(%#[1]v)", u.Object) |
There was a problem hiding this comment.
📝 I found these links helpful
- https://pkg.go.dev/fmt#hdr-Explicit_argument_indexes
- https://faun.pub/golangs-fmt-sprintf-and-printf-demystified-4adf6f9722a2
Also, for reference, a simple %v would yield
map[spec:map[nested:map[field:asdf]]]
| // Correctly fails with: BUG: called without a destination | ||
| // require.UnmarshalIntoField(t, &u, `true`) |
| assert.NilError(t, cc.Create(ctx, cluster, client.DryRunAll)) | ||
| }) | ||
| } | ||
|
|
||
| func TestPostgresUserInterfaceAcrossVersions(t *testing.T) { |
There was a problem hiding this comment.
Would it be worth renaming this file in the future? I like the specificity of the new files' names.
As I was working a change to v1 validation, I wanted to include the v1beta1 tests so as not to introduce any regressions. I wasn't able to share tests using generics because the tests reach into specific struct fields. I was surprised to find a test failure when using unstructured, but the test was right!
So, unstructured seems like an improvement in validation testing overall:
require.UnmarshalIntoFieldorunstructured.SetNestedFieldChecklist:
Type of Changes: