Warn when "log_directory" is outside the v1 range

Issue: PGO-2558

Author: cbandy

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()
@@ -180,7 +180,7 @@ func (*Reconciler) generatePostgresParameters(
 	}
 
 	// Finally, remove or replace harmful values.
-	postgres.SanitizeParameters(cluster, result)
+	postgres.SanitizeParameters(cluster, result, r.Recorder)
 
 	return result
 }

internal/postgres/validation.go

@@ -9,13 +9,17 @@ import (
 	"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) {
+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))
+		parameters.Add("log_directory", sanitizeLogDirectory(cluster, v, recorder))
 	}
 }
 
@@ -52,7 +56,7 @@ var sensitiveRelativePath = regexp.MustCompile(
 // 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) string {
+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.
@@ -69,6 +73,11 @@ func sanitizeLogDirectory(cluster *v1beta1.PostgresCluster, input string) string
 	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) {
@@ -82,6 +91,11 @@ func sanitizeLogDirectory(cluster *v1beta1.PostgresCluster, input string) string
 		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)

internal/postgres/validation_test.go

@@ -10,6 +10,9 @@ import (
 
 	"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"
 )
 
@@ -18,31 +21,61 @@ func TestSanitizeLogDirectory(t *testing.T) {
 
 	cluster := new(v1beta1.PostgresCluster)
 	cluster.Spec.PostgresVersion = 18
+	cluster.UID = "doot"
 
-	for _, tt := range []struct{ expected, from string }{
+	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: ""},
-		{expected: "/pgdata/logs/postgres", from: "/"},
+		{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: "."},
-		{expected: "/pgdata/logs/postgres", from: "global"},
-		{expected: "/pgdata/logs/postgres", from: "postgresql.conf"},
-		{expected: "/pgdata/logs/postgres", from: "postgresql.auto.conf"},
-		{expected: "/pgdata/logs/postgres", from: "pg_wal"},
-		{expected: "/pgdata/logs/postgres", from: "/pgdata/pg99/any"},
-		{expected: "/pgdata/logs/postgres", from: "/pgdata/pg18_wal"},
-		{expected: "/pgdata/logs/postgres", from: "/pgdata/pgsql_tmp/1/2"},
-		{expected: "/pgwal/logs/postgres", from: "/pgwal/pg18_wal"},
-		{expected: "/pgtmp/logs/postgres", from: "/pgtmp/pg18_wal"},
+		{
+			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"`,
+		},
 	} {
-		result := sanitizeLogDirectory(cluster, tt.from)
+		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))
+		}
 	}
 }

internal/text/text.go

@@ -0,0 +1,11 @@
+// Copyright 2025 Crunchy Data Solutions, Inc.
+//
+// SPDX-License-Identifier: Apache-2.0
+
+package text
+
+// TruncateAt returns the first n bytes of s.
+// It returns empty string when s is empty or n is less than one.
+func TruncateAt(s string, n int) string {
+	return s[:max(0, min(n, len(s)))]
+}

internal/text/text_test.go

@@ -0,0 +1,27 @@
+// Copyright 2025 Crunchy Data Solutions, Inc.
+//
+// SPDX-License-Identifier: Apache-2.0
+
+package text_test
+
+import (
+	"testing"
+
+	"gotest.tools/v3/assert"
+
+	"github.com/crunchydata/postgres-operator/internal/text"
+)
+
+func TestTruncateAt(t *testing.T) {
+	assert.Equal(t, text.TruncateAt("", -5), "")
+	assert.Equal(t, text.TruncateAt("", -0), "")
+	assert.Equal(t, text.TruncateAt("", +0), "")
+	assert.Equal(t, text.TruncateAt("", +5), "")
+
+	assert.Equal(t, text.TruncateAt("lorem ipsum dolor sit amet", -5), "")
+	assert.Equal(t, text.TruncateAt("lorem ipsum dolor sit amet", -0), "")
+	assert.Equal(t, text.TruncateAt("lorem ipsum dolor sit amet", +0), "")
+	assert.Equal(t, text.TruncateAt("lorem ipsum dolor sit amet", +5), "lorem")
+	assert.Equal(t, text.TruncateAt("lorem ipsum dolor sit amet", 15), "lorem ipsum dol")
+	assert.Equal(t, text.TruncateAt("lorem ipsum dolor sit amet", 50), "lorem ipsum dolor sit amet")
+}