Make the default Postgres log directory outside its data directory

.github/workflows/test.yaml

@@ -53,7 +53,7 @@ jobs:
     strategy:
       fail-fast: false
       matrix:
-        kubernetes: [v1.33, v1.28]
+        kubernetes: [v1.30, v1.33]
     steps:
       - uses: actions/checkout@v5
       - uses: actions/setup-go@v6
@@ -87,7 +87,7 @@ jobs:
     strategy:
       fail-fast: false
       matrix:
-        kubernetes: [v1.33, v1.28]
+        kubernetes: [v1.30, v1.33]
     steps:
       - uses: actions/checkout@v5
       - uses: actions/setup-go@v6

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

@@ -4996,6 +4996,13 @@ spec:
                       rule: '!has(self.logging_collector)'
                     - message: log_file_mode cannot be changed
                       rule: '!has(self.log_file_mode)'
+                    - fieldPath: .log_directory
+                      message: must start with "/pgdata/logs/postgres", "/pgtmp/logs/postgres",
+                        "/pgwal/logs/postgres", "/volumes", or be "log" to keep logs
+                        inside PGDATA
+                      rule: self.?log_directory.optMap(v, type(v) == string && (v
+                        == "log" || v.startsWith("/volumes") || ["/pgdata","/pgtmp","/pgwal","/volumes"].exists(p,
+                        v == (p + "/logs/postgres") || v.startsWith(p + "/logs/postgres/")))).orValue(true)
                 type: object
               customReplicationTLSSecret:
                 description: |-
@@ -18649,6 +18656,21 @@ spec:
             - instances
             - postgresVersion
             type: object
+            x-kubernetes-validations:
+            - fieldPath: .config.parameters.log_directory
+              message: all instances need "volumes.temp" to log in "/pgtmp"
+              rule: self.?config.parameters.log_directory.optMap(v, type(v) != string
+                || !v.startsWith("/pgtmp/logs/postgres") || self.instances.all(i,
+                i.?volumes.temp.hasValue())).orValue(true)
+            - fieldPath: .config.parameters.log_directory
+              message: all instances need "walVolumeClaimSpec" to log in "/pgwal"
+              rule: self.?config.parameters.log_directory.optMap(v, type(v) != string
+                || !v.startsWith("/pgwal/logs/postgres") || self.instances.all(i,
+                i.?walVolumeClaimSpec.hasValue())).orValue(true)
+            - fieldPath: .config.parameters.log_directory
+              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)
           status:
             description: PostgresClusterStatus defines the observed state of PostgresCluster
             properties:

internal/collector/instance.go

@@ -181,11 +181,8 @@ func AddToPod(
 // startCommand generates the command script used by the collector container
 func startCommand(logDirectories []string, includeLogrotate bool) []string {
 	var mkdirScript string
-	if len(logDirectories) != 0 {
-		for _, logDir := range logDirectories {
-			mkdirScript = mkdirScript + `
-` + shell.MakeDirectories(logDir, "receiver")
-		}
+	for _, logDir := range logDirectories {
+		mkdirScript += "\n" + shell.MakeDirectories(logDir, "receiver")
 	}
 
 	var logrotateCommand string

internal/collector/postgres.go

@@ -64,9 +64,6 @@ func PostgreSQLParameters(ctx context.Context,
 
 		// Log in a timezone the OpenTelemetry Collector understands.
 		outParameters.Mandatory.Add("log_timezone", "UTC")
-
-		// TODO(logs): Remove this call and do it in [postgres.NewParameters] regardless of the gate.
-		outParameters.Mandatory.Add("log_directory", fmt.Sprintf("%s/logs/postgres", postgres.DataStorage(inCluster)))
 	}
 }
 

internal/collector/postgres_test.go

@@ -33,9 +33,7 @@ func TestEnablePostgresLogging(t *testing.T) {
 		config := NewConfig(nil)
 		params := postgres.NewParameters()
 
-		// NOTE: This call is necessary only because the default "log_directory" is not absolute.
-		PostgreSQLParameters(ctx, cluster, &params)
-		EnablePostgresLogging(ctx, cluster, params.Mandatory, config)
+		EnablePostgresLogging(ctx, cluster, params.Default, config)
 
 		result, err := config.ToYAML()
 		assert.NilError(t, err)
@@ -297,9 +295,7 @@ service:
 		config := NewConfig(cluster.Spec.Instrumentation)
 		params := postgres.NewParameters()
 
-		// NOTE: This call is necessary only because the default "log_directory" is not absolute.
-		PostgreSQLParameters(ctx, cluster, &params)
-		EnablePostgresLogging(ctx, cluster, params.Mandatory, config)
+		EnablePostgresLogging(ctx, cluster, params.Default, config)
 
 		result, err := config.ToYAML()
 		assert.NilError(t, err)

internal/controller/postgrescluster/instance.go

@@ -1194,6 +1194,10 @@ func (r *Reconciler) reconcileInstance(
 		// set includeLogrotate to true, but only if backups are enabled
 		// and local volumes are available.
 		includeLogrotate := backupsSpecFound && pgbackrest.RepoHostVolumeDefined(cluster)
+
+		// The log directories here do not include Postgres "log_directory" because it might be a subdirectory of "data_directory".
+		// In that case, the "log_directory" must be created *after* initdb runs, not during container startup here.
+		// TODO(sidecar): Create these directories sometime other than startup.
 		collector.AddToPod(ctx, cluster.Spec.Instrumentation, cluster.Spec.ImagePullPolicy, instanceConfigMap, &instance.Spec.Template,
 			[]corev1.VolumeMount{postgres.DataVolumeMount()}, pgPassword,
 			[]string{naming.PGBackRestPGDataLogPath}, includeLogrotate, true)

internal/controller/postgrescluster/postgres.go

@@ -126,7 +126,7 @@ func (r *Reconciler) generatePostgresHBAs(
 //  2. parameters in cluster.spec.config.parameters
 //  3. parameters in cluster.spec.patroni.dynamicConfiguration
 //  4. default values determined by contollers
-func (*Reconciler) generatePostgresParameters(
+func (r *Reconciler) generatePostgresParameters(
 	ctx context.Context, cluster *v1beta1.PostgresCluster, backupsSpecFound bool,
 ) *postgres.ParameterSet {
 	builtin := postgres.NewParameters()
@@ -179,6 +179,9 @@ func (*Reconciler) generatePostgresParameters(
 		result.Add("shared_preload_libraries", preload)
 	}
 
+	// Finally, remove or replace harmful values.
+	postgres.SanitizeParameters(cluster, result, r.Recorder)
+
 	return result
 }
 

internal/controller/postgrescluster/postgres_test.go

@@ -181,15 +181,19 @@ func TestGeneratePostgresParameters(t *testing.T) {
 			parameters: {
 				something: str,
 				another: 5,
+				log_directory: pg_wal,
 			},
 		}`)
 
 		result := reconciler.generatePostgresParameters(ctx, cluster, false)
 		assert.Assert(t, cmp.LenMap(result.AsMap(), len(builtin.AsMap())+2),
-			"expected two parameters from the Config section")
+			"expected two new parameters from the Config section")
 
 		assert.Equal(t, result.Value("another"), "5")
 		assert.Equal(t, result.Value("something"), "str")
+
+		assert.Equal(t, result.Value("log_directory"), "/pgdata/logs/postgres",
+			"expected sanitization")
 	})
 
 	t.Run("Patroni", func(t *testing.T) {

internal/postgres/parameters.go

@@ -7,6 +7,7 @@ package postgres
 import (
 	"fmt"
 	"maps"
+	"path"
 	"slices"
 	"strings"
 )
@@ -48,16 +49,13 @@ func NewParameters() Parameters {
 	// - https://www.postgresql.org/docs/current/auth-password.html
 	parameters.Default.Add("password_encryption", "scram-sha-256")
 
-	// Explicitly use Postgres' default log directory. This value is relative to the "data_directory" parameter.
+	// Log outside of the Postgres data directory by default.
 	//
-	// Controllers look at this parameter to grant group-write S_IWGRP on the directory.
-	// Postgres does not grant this permission on the directories it creates.
-	//
-	// TODO(logs): A better default would be *outside* the data directory.
-	// This parameter needs to be configurable and documented before the default can change.
+	// When log files are inside the data directory, they are destroyed along with the data directory
+	// during replica creation and major upgrades. Being outside also reduces the size of backups.
 	//
 	// PostgreSQL must be reloaded when changing this parameter.
-	parameters.Default.Add("log_directory", "log")
+	parameters.Default.Add("log_directory", path.Join(dataMountPath, "logs/postgres"))
 
 	// Pod "securityContext.fsGroup" ensures processes and filesystems agree on a GID;
 	// use the same permissions for group and owner.

internal/postgres/parameters_test.go

@@ -28,8 +28,7 @@ func TestNewParameters(t *testing.T) {
 	assert.DeepEqual(t, parameters.Default.AsMap(), map[string]string{
 		"jit": "off",
 
-		"log_directory": "log",
-
+		"log_directory":       "/pgdata/logs/postgres",
 		"password_encryption": "scram-sha-256",
 	})
 }

internal/postgres/reconcile_test.go

@@ -288,9 +288,8 @@ initContainers:
     bootstrap_dir="${postgres_data_directory}_bootstrap"
     [[ -d "${bootstrap_dir}" ]] && postgres_data_directory="${bootstrap_dir}" && results 'bootstrap directory' "${bootstrap_dir}"
     dataDirectory "${postgres_data_directory}" || halt "$(permissions "${postgres_data_directory}" ||:)"
-    [[ ! -f '/pgdata/pg11/PG_VERSION' ]] ||
-    (mkdir -p '/pgdata/pg11/log' && { chmod 0775 '/pgdata/pg11/log' || :; }) ||
-    halt "$(permissions '/pgdata/pg11/log' ||:)"
+    (mkdir -p '/pgdata/logs/postgres' && { chmod 0775 '/pgdata/logs/postgres' '/pgdata/logs' || :; }) ||
+    halt "$(permissions '/pgdata/logs/postgres' ||:)"
     (mkdir -p '/pgdata/patroni/log' && { chmod 0775 '/pgdata/patroni/log' '/pgdata/patroni' || :; }) ||
     halt "$(permissions '/pgdata/patroni/log' ||:)"
     (mkdir -p '/pgdata/pgbackrest/log' && { chmod 0775 '/pgdata/pgbackrest/log' '/pgdata/pgbackrest' || :; }) ||

internal/postgres/validation.go

@@ -0,0 +1,107 @@
+// Copyright 2021 - 2025 Crunchy Data Solutions, Inc.
+//
+// SPDX-License-Identifier: Apache-2.0
+
+package postgres
+
+import (
+	"path"
+	"regexp"
+	"strings"
+
+	corev1 "k8s.io/api/core/v1"
+	"k8s.io/client-go/tools/record"
+
+	"github.com/crunchydata/postgres-operator/internal/text"
+	"github.com/crunchydata/postgres-operator/pkg/apis/postgres-operator.crunchydata.com/v1beta1"
+)
+
+// SanitizeParameters transforms parameters so they are safe for Postgres in cluster.
+func SanitizeParameters(cluster *v1beta1.PostgresCluster, parameters *ParameterSet, recorder record.EventRecorder) {
+	if v, ok := parameters.Get("log_directory"); ok {
+		parameters.Add("log_directory", sanitizeLogDirectory(cluster, v, recorder))
+	}
+}
+
+// sensitiveAbsolutePath matches absolute paths that Postgres expects to control.
+// User input should not direct tools to write to these directories.
+//
+// See [sanitizeLogDirectory].
+var sensitiveAbsolutePath = regexp.MustCompile(
+	// Rooted in one of these volumes
+	`^(` + dataMountPath + `|` + tmpMountPath + `|` + walMountPath + `)` +
+
+		// First subdirectory is a Postgres directory
+		`/(` + `pg\d+` + // [DataDirectory]
+		`|` + `pgsql_tmp` + // https://www.postgresql.org/docs/current/storage-file-layout.html
+		`|` + `pg\d+_wal` + // [WALDirectory]
+		`)(/|$)`,
+)
+
+// sensitiveRelativePath matches paths relative to the Postgres "data_directory" that Postgres expects to control.
+// Arguably, everything inside the data directory is sensitve, but this is here because
+// Postges interprets some of its parameters relative to its data directory.
+//
+// User input should not direct tools to write to these directories.
+//
+// NOTE: This is not an exhaustive list! New code should use an allowlist rather than this denylist.
+//
+// See [sanitizeLogDirectory].
+var sensitiveRelativePath = regexp.MustCompile(
+	`^(archive|base|current|global|patroni|pg_|PG_|postgresql|postmaster|[[:xdigit:]]{24,})` +
+		`|` + `[.](history|partial)$`,
+)
+
+// sanitizeLogDirectory returns the absolute path to input when it is a safe "log_directory" for cluster.
+// Otherwise, it returns the absolute path to a good "log_directory" value.
+//
+// https://www.postgresql.org/docs/current/runtime-config-logging.html#GUC-LOG-DIRECTORY
+func sanitizeLogDirectory(cluster *v1beta1.PostgresCluster, input string, recorder record.EventRecorder) string {
+	directory := path.Clean(input)
+
+	// [path.Clean] leaves leading parent directories. Eliminate these as a security measure.
+	for strings.HasPrefix(directory, "../") {
+		directory = directory[3:]
+	}
+
+	switch {
+	case directory == "log":
+		// This the Postgres default and the only relative path allowed in v1 of PostgresCluster.
+		// Expand it relative to the data directory like Postgres does.
+		return path.Join(DataDirectory(cluster), "log")
+
+	case directory == "", directory == ".", directory == "/",
+		sensitiveAbsolutePath.MatchString(directory),
+		sensitiveRelativePath.MatchString(directory):
+		if recorder != nil {
+			recorder.Eventf(cluster, corev1.EventTypeWarning, "InvalidParameter",
+				"Ignoring unsafe Postgres parameter value %q = %q", "log_directory", text.TruncateAt(input, 128))
+		}
+
+		// When the value is empty after cleaning or disallowed, choose one instead.
+		// Keep it on the same volume, if possible.
+		if strings.HasPrefix(directory, tmpMountPath) {
+			return path.Join(tmpMountPath, "logs/postgres")
+		}
+		if strings.HasPrefix(directory, walMountPath) {
+			return path.Join(walMountPath, "logs/postgres")
+		}
+
+		// There is always a data volume, so use that.
+		return path.Join(dataMountPath, "logs/postgres")
+
+	case !path.IsAbs(directory):
+		if recorder != nil {
+			recorder.Eventf(cluster, corev1.EventTypeWarning, "InvalidParameter",
+				"Postgres parameter %q should be %q or an absolute path", "log_directory", "log")
+		}
+
+		// Directory is relative. This is disallowed since v1 of PostgresCluster.
+		// Expand it relative to the data directory like Postgres does.
+		return path.Join(DataDirectory(cluster), directory)
+
+	default:
+		// Directory is absolute and considered safe; use it.
+		return directory
+	}
+}

internal/postgres/validation_test.go

@@ -0,0 +1,81 @@
+// Copyright 2021 - 2025 Crunchy Data Solutions, Inc.
+//
+// SPDX-License-Identifier: Apache-2.0
+
+package postgres
+
+import (
+	"path"
+	"testing"
+
+	"gotest.tools/v3/assert"
+
+	"github.com/crunchydata/postgres-operator/internal/controller/runtime"
+	"github.com/crunchydata/postgres-operator/internal/testing/cmp"
+	"github.com/crunchydata/postgres-operator/internal/testing/events"
+	"github.com/crunchydata/postgres-operator/pkg/apis/postgres-operator.crunchydata.com/v1beta1"
+)
+
+func TestSanitizeLogDirectory(t *testing.T) {
+	t.Parallel()
+
+	cluster := new(v1beta1.PostgresCluster)
+	cluster.Spec.PostgresVersion = 18
+	cluster.UID = "doot"
+
+	recorder := events.NewRecorder(t, runtime.Scheme)
+
+	for _, tt := range []struct{ expected, from, event string }{
+		// User wants logs inside the data directory.
+		{expected: "/pgdata/pg18/log", from: "log"},
+
+		// Other relative paths warn.
+		{
+			expected: "/pgdata/pg18/some/directory", from: "some/directory",
+			event: `"log_directory" should be "log" or an absolute path`,
+		},
+
+		// Postgres interprets blank to mean root.
+		// That's no good, so we choose better.
+		{expected: "/pgdata/logs/postgres", from: "", event: `"log_directory" = ""`},
+		{expected: "/pgdata/logs/postgres", from: "/", event: `"log_directory" = "/"`},
+
+		// Paths into Postgres directories are replaced on the same volume.
+		{
+			expected: "/pgdata/logs/postgres", from: ".", event: `"log_directory" = "."`,
+		}, {
+			expected: "/pgdata/logs/postgres", from: "global", event: `"log_directory" = "global"`,
+		}, {
+			expected: "/pgdata/logs/postgres", from: "postgresql.conf", event: `"log_directory" = "postgresql.conf"`,
+		}, {
+			expected: "/pgdata/logs/postgres", from: "postgresql.auto.conf", event: `"log_directory" = "postgresql.auto.conf"`,
+		}, {
+			expected: "/pgdata/logs/postgres", from: "pg_wal", event: `"log_directory" = "pg_wal"`,
+		}, {
+			expected: "/pgdata/logs/postgres", from: "/pgdata/pg99/any", event: `"log_directory" = "/pgdata/pg99/any"`,
+		}, {
+			expected: "/pgdata/logs/postgres", from: "/pgdata/pg18_wal", event: `"log_directory" = "/pgdata/pg18_wal"`,
+		}, {
+			expected: "/pgdata/logs/postgres", from: "/pgdata/pgsql_tmp/ludicrous/speed", event: `"log_directory" = "/pgdata/pgsql_tmp/ludicrous/speed"`,
+		}, {
+			expected: "/pgwal/logs/postgres", from: "/pgwal/pg18_wal", event: `"log_directory" = "/pgwal/pg18_wal"`,
+		}, {
+			expected: "/pgtmp/logs/postgres", from: "/pgtmp/pg18_wal", event: `"log_directory" = "/pgtmp/pg18_wal"`,
+		},
+	} {
+		recorder.Events = nil
+		result := sanitizeLogDirectory(cluster, tt.from, recorder)
+
+		assert.Equal(t, tt.expected, result, "from: %s", tt.from)
+		assert.Assert(t, path.IsAbs(result))
+
+		if len(tt.event) > 0 {
+			assert.Assert(t, cmp.Len(recorder.Events, 1))
+			assert.Equal(t, recorder.Events[0].Type, "Warning")
+			assert.Equal(t, recorder.Events[0].Reason, "InvalidParameter")
+			assert.Equal(t, recorder.Events[0].Regarding.Kind, "PostgresCluster")
+			assert.Assert(t, cmp.Equal(recorder.Events[0].Regarding.UID, "doot"))
+			assert.Assert(t, cmp.Contains(recorder.Events[0].Note, tt.event))
+		}
+	}
+}