Always set Postgres "log_file_mode" parameter

We set it when OpenTelemetry is enabled, but group permissions are good
on Kubernetes storage generally. This adds some more validation tests
around Postgres logging parameters.

Issue: PGO-2558

Author: cbandy

internal/collector/postgres.go

@@ -117,7 +117,6 @@ func EnablePostgresLogging(
 		for k, v := range postgres.LogRotation(retentionPeriod, "postgresql-", ".log") {
 			outParameters.Add(k, v)
 		}
-		outParameters.Add("log_file_mode", "0660")
 
 		// Log in a timezone that the OpenTelemetry Collector will understand.
 		outParameters.Add("log_timezone", "UTC")

internal/postgres/parameters.go

@@ -48,6 +48,16 @@ func NewParameters() Parameters {
 	// - https://www.postgresql.org/docs/current/auth-password.html
 	parameters.Default.Add("password_encryption", "scram-sha-256")
 
+	// Pod "securityContext.fsGroup" ensures processes and filesystems agree on a GID;
+	// use the same permissions for group and owner.
+	// This allows every process in the pod to read Postgres log files.
+	//
+	// S_IRUSR, S_IWUSR: (0600) enable owner read and write permissions
+	// S_IRGRP, S_IWGRP: (0060) enable group read and write permissions.
+	//
+	// PostgreSQL must be reloaded when changing this value.
+	parameters.Mandatory.Add("log_file_mode", "0660")
+
 	return parameters
 }
 

internal/postgres/parameters_test.go

@@ -14,6 +14,8 @@ func TestNewParameters(t *testing.T) {
 	parameters := NewParameters()
 
 	assert.DeepEqual(t, parameters.Mandatory.AsMap(), map[string]string{
+		"log_file_mode": "0660",
+
 		"ssl":           "on",
 		"ssl_ca_file":   "/pgconf/tls/ca.crt",
 		"ssl_cert_file": "/pgconf/tls/tls.crt",

internal/shell/paths.go

@@ -11,6 +11,7 @@ import (
 	"fmt"
 	"io/fs"
 	"path/filepath"
+	"slices"
 	"strings"
 )
 
@@ -41,17 +42,15 @@ func CleanFileName(path string) string {
 //   - https://pubs.opengroup.org/onlinepubs/9799919799/utilities/test.html
 //   - https://pubs.opengroup.org/onlinepubs/9799919799/utilities/umask.html
 func MakeDirectories(base string, paths ...string) string {
-	// Without any paths, return a command that succeeds when the base path
-	// exists.
+	// Without any paths, return a command that succeeds when the base path exists.
 	if len(paths) == 0 {
 		return `test -d ` + QuoteWord(base)
 	}
 
-	allPaths := append([]string(nil), paths...)
+	allPaths := slices.Clone(paths)
 	for _, p := range paths {
 		if r, err := filepath.Rel(base, p); err == nil && filepath.IsLocal(r) {
-			// The result of [filepath.Rel] is a shorter representation
-			// of the full path; skip it.
+			// The result of [filepath.Rel] is a shorter representation of the full path; skip it.
 			r = filepath.Dir(r)
 
 			for r != "." {
@@ -61,6 +60,8 @@ func MakeDirectories(base string, paths ...string) string {
 		}
 	}
 
+	// Pod "securityContext.fsGroup" ensures processes and filesystems agree on a GID.
+	// Use the same permissions for group and owner.
 	const perms fs.FileMode = 0 |
 		// S_IRWXU: enable owner read, write, and execute permissions.
 		0o0700 |

internal/shell/paths_test.go

@@ -93,4 +93,11 @@ func TestMakeDirectories(t *testing.T) {
 				"expected plain unquoted scalar, got:\n%s", b)
 		})
 	})
+
+	t.Run("Unrelated", func(t *testing.T) {
+		assert.Equal(t,
+			MakeDirectories("/one", "/two/three/four"),
+			`mkdir -p '/two/three/four' && { chmod 0775 '/two/three/four' || :; }`,
+			"expected no chmod of parent directories")
+	})
 }

internal/testing/validation/postgrescluster/postgres_config_test.go

@@ -5,6 +5,7 @@
 package validation
 
 import (
+	"fmt"
 	"testing"
 
 	"gotest.tools/v3/assert"
@@ -118,8 +119,6 @@ func testPostgresConfigParametersCommon(t *testing.T, cc client.Client, base uns
 			{key: "hot_standby", value: "off"},
 			{key: "ident_file", value: "two"},
 			{key: "listen_addresses", value: ""},
-			{key: "log_file_mode", value: ""},
-			{key: "logging_collector", value: "off"},
 			{key: "port", value: 5},
 			{key: "wal_log_hints", value: "off"},
 		} {
@@ -143,6 +142,57 @@ func testPostgresConfigParametersCommon(t *testing.T, cc client.Client, base uns
 		}
 	})
 
+	t.Run("Logging", func(t *testing.T) {
+		for _, tt := range []struct {
+			valid   bool
+			key     string
+			value   any
+			message string
+		}{
+			{valid: false, key: "log_file_mode", value: "", message: "cannot be changed"},
+			{valid: false, key: "log_file_mode", value: "any", message: "cannot be changed"},
+			{valid: false, key: "logging_collector", value: "", message: "unsafe"},
+			{valid: false, key: "logging_collector", value: "off", message: "unsafe"},
+			{valid: false, key: "logging_collector", value: "on", message: "unsafe"},
+
+			{valid: true, key: "log_destination", value: "anything"},
+			{valid: true, key: "log_directory", value: "anything"},
+			{valid: true, key: "log_filename", value: "anything"},
+			{valid: true, key: "log_filename", value: "percent-%s-too"},
+			{valid: true, key: "log_rotation_age", value: "7d"},
+			{valid: true, key: "log_rotation_age", value: 5},
+			{valid: true, key: "log_rotation_size", value: "100MB"},
+			{valid: true, key: "log_rotation_size", value: 13},
+			{valid: true, key: "log_timezone", value: ""},
+			{valid: true, key: "log_timezone", value: "nonsense"},
+		} {
+			t.Run(fmt.Sprint(tt), func(t *testing.T) {
+				cluster := base.DeepCopy()
+				require.UnmarshalIntoField(t, cluster,
+					require.Value(yaml.Marshal(tt.value)),
+					"spec", "config", "parameters", tt.key)
+
+				err := cc.Create(ctx, cluster, client.DryRunAll)
+
+				if tt.valid {
+					assert.NilError(t, err)
+					assert.Equal(t, "", tt.message, "BUG IN TEST: no message expected when valid")
+				} else {
+					assert.Assert(t, apierrors.IsInvalid(err))
+
+					status := require.StatusError(t, err)
+					assert.Assert(t, status.Details != nil)
+					assert.Assert(t, cmp.Len(status.Details.Causes, 1))
+
+					// TODO(k8s-1.30) TODO(validation): Move the parameter name from the message to the field path.
+					assert.Equal(t, status.Details.Causes[0].Field, "spec.config.parameters")
+					assert.Assert(t, cmp.Contains(status.Details.Causes[0].Message, tt.key))
+					assert.Assert(t, cmp.Contains(status.Details.Causes[0].Message, tt.message))
+				}
+			})
+		}
+	})
+
 	t.Run("NoConnections", func(t *testing.T) {
 		for _, tt := range []struct {
 			key   string