Add validation rule that ensures that all instances have an additional volume when attempting to log pgbackrest sidecar to /volumes. Add test cases.

Author: dsessler7

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

@@ -18453,6 +18453,11 @@ spec:
               message: all instances need an additional volume to log in "/volumes"
               rule: self.?config.parameters.log_directory.optMap(v, type(v) != string
                 || !v.startsWith("/volumes") || self.instances.all(i, i.?volumes.additional.hasValue())).orValue(true)
+            - fieldPath: .backups.pgbackrest.log.path
+              message: all instances need an additional volume for pgbackrest sidecar
+                to log in "/volumes"
+              rule: self.?backups.pgbackrest.log.path.optMap(v, !v.startsWith("/volumes")
+                || self.instances.all(i, i.?volumes.additional.hasValue())).orValue(true)
           status:
             description: PostgresClusterStatus defines the observed state of PostgresCluster
             properties:

internal/testing/validation/pgbackrest_test.go

@@ -63,23 +63,106 @@ func TestV1PGBackRestLogging(t *testing.T) {
 	t.Run("Cannot set pgbackrest sidecar's log.path without correct subdir", func(t *testing.T) {
 		tmp := base.DeepCopy()
 
-		require.UnmarshalInto(t, &tmp.Spec.Backups.PGBackRest, `{
-			log: {
-				path: "/something/wrong"
-			}
-		}`)
+		t.Run("Wrong subdir", func(t *testing.T) {
+			require.UnmarshalInto(t, &tmp.Spec.Backups.PGBackRest, `{
+				log: {
+					path: "/something/wrong"
+				}
+			}`)
 
-		err := cc.Create(ctx, tmp.DeepCopy(), client.DryRunAll)
-		assert.Assert(t, apierrors.IsInvalid(err))
-		assert.ErrorContains(t, err, "pgbackrest sidecar log path is restricted to an existing additional volume")
+			err := cc.Create(ctx, tmp.DeepCopy(), client.DryRunAll)
+			assert.Assert(t, apierrors.IsInvalid(err))
+			assert.ErrorContains(t, err, "pgbackrest sidecar log path is restricted to an existing additional volume")
+		})
 
-		require.UnmarshalInto(t, &tmp.Spec.Backups.PGBackRest, `{
-			log: {
-				path: "/volumes/this/should/pass"
-			}
-		}`)
+		t.Run("Single instance - missing additional volume", func(t *testing.T) {
+			require.UnmarshalInto(t, &tmp.Spec.Backups.PGBackRest, `{
+				log: {
+					path: "/volumes/anything"
+				}
+			}`)
 
-		assert.NilError(t, cc.Create(ctx, tmp.DeepCopy(), client.DryRunAll), "expected log.path to be valid")
+			err := cc.Create(ctx, tmp.DeepCopy(), client.DryRunAll)
+			assert.Assert(t, apierrors.IsInvalid(err))
+			assert.ErrorContains(t, err, `all instances need an additional volume for pgbackrest sidecar to log in "/volumes"`)
+		})
+
+		t.Run("Multiple instances - one missing additional volume", func(t *testing.T) {
+			require.UnmarshalInto(t, &tmp.Spec.Backups.PGBackRest, `{
+				log: {
+					path: "/volumes/anything"
+				}
+			}`)
+
+			require.UnmarshalInto(t, &tmp.Spec.InstanceSets, `[{
+				dataVolumeClaimSpec: {
+					accessModes: [ReadWriteOnce],
+					resources: { requests: { storage: 1Mi } },
+				},
+				volumes: {
+					additional: [{
+						name: "test",
+						claimName: "pvc-claim"
+					}]
+				}
+			},{
+				name: "instance2",
+				dataVolumeClaimSpec: {
+					accessModes: [ReadWriteOnce],
+					resources: { requests: { storage: 1Mi } },
+				},
+			}]`)
+
+			err := cc.Create(ctx, tmp.DeepCopy(), client.DryRunAll)
+			assert.Assert(t, apierrors.IsInvalid(err))
+			assert.ErrorContains(t, err, `all instances need an additional volume for pgbackrest sidecar to log in "/volumes"`)
+		})
+
+		t.Run("Single instance - additional volume present", func(t *testing.T) {
+			require.UnmarshalInto(t, &tmp.Spec.InstanceSets, `[{
+				dataVolumeClaimSpec: {
+					accessModes: [ReadWriteOnce],
+					resources: { requests: { storage: 1Mi } },
+				},
+				volumes: {
+					additional: [{
+						name: "test",
+						claimName: "pvc-claim"
+					}]
+				}
+			}]`)
+
+			assert.NilError(t, cc.Create(ctx, tmp.DeepCopy(), client.DryRunAll), "expected log.path to be valid")
+		})
+
+		t.Run("Multiple instances - additional volume present", func(t *testing.T) {
+			require.UnmarshalInto(t, &tmp.Spec.InstanceSets, `[{
+				dataVolumeClaimSpec: {
+					accessModes: [ReadWriteOnce],
+					resources: { requests: { storage: 1Mi } },
+				},
+				volumes: {
+					additional: [{
+						name: "test",
+						claimName: "pvc-claim"
+					}]
+				}
+			},{
+				name: "instance2",
+				dataVolumeClaimSpec: {
+					accessModes: [ReadWriteOnce],
+					resources: { requests: { storage: 1Mi } },
+				},
+				volumes: {
+					additional: [{
+						name: "another",
+						claimName: "another-pvc-claim"
+					}]
+				}
+			}]`)
+
+			assert.NilError(t, cc.Create(ctx, tmp.DeepCopy(), client.DryRunAll), "expected log.path to be valid")
+		})
 	})
 
 	t.Run("Cannot set logging on volumes that don't exist", func(t *testing.T) {

pkg/apis/postgres-operator.crunchydata.com/v1/postgrescluster_types.go

@@ -26,6 +26,13 @@ import (
 //
 // TODO: Also check the above path against volume names: `i.?volumes.additional.hasValue() && i.volumes.additional.exists(directory.startsWith("/volumes/" + volume.name))`
 // https://github.com/kubernetes-sigs/controller-tools/pull/1270#issuecomment-3272211184
+//
+// # pgBackRest Logging
+//
+// +kubebuilder:validation:XValidation:fieldPath=`.backups.pgbackrest.log.path`,message=`all instances need an additional volume for pgbackrest sidecar to log in "/volumes"`,rule=`self.?backups.pgbackrest.log.path.optMap(v, !v.startsWith("/volumes") || self.instances.all(i, i.?volumes.additional.hasValue())).orValue(true)`
+//
+// TODO: Also check the above path against volume names: `i.?volumes.additional.hasValue() && i.volumes.additional.exists(a, v.startsWith("/volumes/" + a.name))`
+// https://github.com/kubernetes-sigs/controller-tools/pull/1270#issuecomment-3272211184
 type PostgresClusterSpec struct {
 	// +optional
 	Metadata *v1beta1.Metadata `json:"metadata,omitempty"`