diff --git a/internal/collector/postgres.go b/internal/collector/postgres.go index 74fe5cf05..892748c0a 100644 --- a/internal/collector/postgres.go +++ b/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") diff --git a/internal/postgres/parameters.go b/internal/postgres/parameters.go index 469eef0bf..6fb7b0d2f 100644 --- a/internal/postgres/parameters.go +++ b/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 } diff --git a/internal/postgres/parameters_test.go b/internal/postgres/parameters_test.go index 5126899d9..ad8c6e90c 100644 --- a/internal/postgres/parameters_test.go +++ b/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", diff --git a/internal/shell/paths.go b/internal/shell/paths.go index 94c997f7b..701144694 100644 --- a/internal/shell/paths.go +++ b/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 | diff --git a/internal/shell/paths_test.go b/internal/shell/paths_test.go index e723e4006..b5adb69b1 100644 --- a/internal/shell/paths_test.go +++ b/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") + }) } diff --git a/internal/testing/validation/postgrescluster/postgres_config_test.go b/internal/testing/validation/postgrescluster/postgres_config_test.go index a55d8de03..b03ed2971 100644 --- a/internal/testing/validation/postgrescluster/postgres_config_test.go +++ b/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