From b7e0145595d91c4dfaf3a89b02d1c52f84144dab Mon Sep 17 00:00:00 2001 From: Chris Bandy Date: Mon, 4 Nov 2024 09:40:09 -0600 Subject: [PATCH 1/5] Add shared functions for quoting shell words --- internal/patroni/config.go | 19 +++++-------------- internal/shell/quote.go | 34 ++++++++++++++++++++++++++++++++++ internal/shell/quote_test.go | 34 ++++++++++++++++++++++++++++++++++ 3 files changed, 73 insertions(+), 14 deletions(-) create mode 100644 internal/shell/quote.go create mode 100644 internal/shell/quote_test.go diff --git a/internal/patroni/config.go b/internal/patroni/config.go index 63ac9e0617..2961e651d3 100644 --- a/internal/patroni/config.go +++ b/internal/patroni/config.go @@ -15,6 +15,7 @@ import ( "github.com/crunchydata/postgres-operator/internal/config" "github.com/crunchydata/postgres-operator/internal/naming" "github.com/crunchydata/postgres-operator/internal/postgres" + "github.com/crunchydata/postgres-operator/internal/shell" "github.com/crunchydata/postgres-operator/pkg/apis/postgres-operator.crunchydata.com/v1beta1" ) @@ -34,12 +35,6 @@ const ( "# Your changes will not be saved.\n" ) -// quoteShellWord ensures that s is interpreted by a shell as single word. -func quoteShellWord(s string) string { - // https://www.gnu.org/software/bash/manual/html_node/Quoting.html - return `'` + strings.ReplaceAll(s, `'`, `'"'"'`) + `'` -} - // clusterYAML returns Patroni settings that apply to the entire cluster. func clusterYAML( cluster *v1beta1.PostgresCluster, @@ -581,15 +576,11 @@ func instanceYAML( "-", }, command...) - quoted := make([]string, len(command)) - for i := range command { - quoted[i] = quoteShellWord(command[i]) - } postgresql[pgBackRestCreateReplicaMethod] = map[string]any{ - "command": strings.Join(quoted, " "), - "keep_data": true, - "no_leader": true, - "no_params": true, + "command": strings.Join(shell.QuoteWords(command...), " "), + "keep_data": true, // Use the data directory from a prior method. + "no_leader": true, // Works without a replication connection. + "no_params": true, // Patroni should not add "--scope", "--role", etc. } methods = append([]string{pgBackRestCreateReplicaMethod}, methods...) } diff --git a/internal/shell/quote.go b/internal/shell/quote.go new file mode 100644 index 0000000000..bac8d14f93 --- /dev/null +++ b/internal/shell/quote.go @@ -0,0 +1,34 @@ +// Copyright 2024 - 2025 Crunchy Data Solutions, Inc. +// +// SPDX-License-Identifier: Apache-2.0 + +package shell + +import "strings" + +// escapeSingleQuoted is used by [QuoteWord]. +var escapeSingleQuoted = strings.NewReplacer( + // slightly shorter results for the unlikely pair of quotes. + `''`, `'"''"'`, + + // first, close the single-quote U+0027, + // add one between double-quotes U+0022, + // then reopen the single-quote U+0027. + `'`, `'"'"'`, +).Replace + +// QuoteWord ensures that v is interpreted by a shell as a single word. +func QuoteWord(v string) string { + // https://pubs.opengroup.org/onlinepubs/9799919799/utilities/V3_chap02.html + // https://www.gnu.org/software/bash/manual/html_node/Quoting.html + return `'` + escapeSingleQuoted(v) + `'` +} + +// QuoteWords ensures that s is interpreted by a shell as individual words. +func QuoteWords(s ...string) []string { + quoted := make([]string, len(s)) + for i := range s { + quoted[i] = QuoteWord(s[i]) + } + return quoted +} diff --git a/internal/shell/quote_test.go b/internal/shell/quote_test.go new file mode 100644 index 0000000000..eaea72f673 --- /dev/null +++ b/internal/shell/quote_test.go @@ -0,0 +1,34 @@ +// Copyright 2024 - 2025 Crunchy Data Solutions, Inc. +// +// SPDX-License-Identifier: Apache-2.0 + +package shell + +import ( + "testing" + + "gotest.tools/v3/assert" +) + +func TestQuoteWord(t *testing.T) { + assert.Equal(t, QuoteWord(""), `''`, + "expected empty and single-quoted") + + assert.Equal(t, QuoteWord("abc"), `'abc'`, + "expected single-quoted") + + assert.Equal(t, QuoteWord(`a" b"c`), `'a" b"c'`, + "expected easy double-quotes") + + assert.Equal(t, QuoteWord(`a' b'c`), + `'a'`+`"'"`+`' b'`+`"'"`+`'c'`, + "expected close-quote-open twice") + + assert.Equal(t, QuoteWord(`a''b`), + `'a'`+`"''"`+`'b'`, + "expected close-quotes-open once") + + assert.Equal(t, QuoteWord(`x''''y`), + `'x'`+`"''"`+`''`+`"''"`+`'y'`, + "expected close-quotes-open twice") +} From 1aaa941b2166ec8f3700e6c39b6899ffa9d25890 Mon Sep 17 00:00:00 2001 From: Chris Bandy Date: Mon, 10 Feb 2025 09:40:47 -0600 Subject: [PATCH 2/5] Add a function for setting permission on directories --- internal/shell/paths.go | 57 +++++++++++++++++++++++++++++++ internal/shell/paths_test.go | 66 ++++++++++++++++++++++++++++++++++++ 2 files changed, 123 insertions(+) create mode 100644 internal/shell/paths.go create mode 100644 internal/shell/paths_test.go diff --git a/internal/shell/paths.go b/internal/shell/paths.go new file mode 100644 index 0000000000..3455ff8fe4 --- /dev/null +++ b/internal/shell/paths.go @@ -0,0 +1,57 @@ +// Copyright 2024 - 2025 Crunchy Data Solutions, Inc. +// +// SPDX-License-Identifier: Apache-2.0 + +// We want the [filepath] package to behave correctly for Linux containers. +//go:build unix + +package shell + +import ( + "fmt" + "io/fs" + "path/filepath" + "strings" +) + +// MakeDirectories returns a list of POSIX shell commands that ensure each path +// exists. It creates every directory leading to path from (but not including) +// base and sets their permissions to exactly perms, regardless of umask. +// +// See: +// - https://pubs.opengroup.org/onlinepubs/9799919799/utilities/chmod.html +// - https://pubs.opengroup.org/onlinepubs/9799919799/utilities/mkdir.html +// - https://pubs.opengroup.org/onlinepubs/9799919799/utilities/test.html +// - https://pubs.opengroup.org/onlinepubs/9799919799/utilities/umask.html +func MakeDirectories(perms fs.FileMode, base string, paths ...string) string { + // 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...) + 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. + r = filepath.Dir(r) + + for r != "." { + allPaths = append(allPaths, filepath.Join(base, r)) + r = filepath.Dir(r) + } + } + } + + return `` + + // Create all the paths and any missing parents. + `mkdir -p ` + strings.Join(QuoteWords(paths...), " ") + + + // Set the permissions of every path and each parent. + // NOTE: FileMode bits other than file permissions are ignored. + fmt.Sprintf(` && chmod %#o %s`, + perms&fs.ModePerm, + strings.Join(QuoteWords(allPaths...), " "), + ) +} diff --git a/internal/shell/paths_test.go b/internal/shell/paths_test.go new file mode 100644 index 0000000000..273f672b79 --- /dev/null +++ b/internal/shell/paths_test.go @@ -0,0 +1,66 @@ +// Copyright 2024 - 2025 Crunchy Data Solutions, Inc. +// +// SPDX-License-Identifier: Apache-2.0 + +package shell + +import ( + "os" + "os/exec" + "path/filepath" + "strings" + "testing" + + "gotest.tools/v3/assert" + "sigs.k8s.io/yaml" + + "github.com/crunchydata/postgres-operator/internal/testing/require" +) + +func TestMakeDirectories(t *testing.T) { + t.Parallel() + + t.Run("NoPaths", func(t *testing.T) { + assert.Equal(t, + MakeDirectories(0o755, "/asdf/jklm"), + `test -d '/asdf/jklm'`) + }) + + t.Run("Children", func(t *testing.T) { + assert.DeepEqual(t, + MakeDirectories(0o775, "/asdf", "/asdf/jklm", "/asdf/qwerty"), + `mkdir -p '/asdf/jklm' '/asdf/qwerty' && chmod 0775 '/asdf/jklm' '/asdf/qwerty'`) + }) + + t.Run("Grandchild", func(t *testing.T) { + script := MakeDirectories(0o775, "/asdf", "/asdf/qwerty/boots") + assert.DeepEqual(t, script, + `mkdir -p '/asdf/qwerty/boots' && chmod 0775 '/asdf/qwerty/boots' '/asdf/qwerty'`) + + t.Run("ShellCheckPOSIX", func(t *testing.T) { + shellcheck := require.ShellCheck(t) + + dir := t.TempDir() + file := filepath.Join(dir, "script.sh") + assert.NilError(t, os.WriteFile(file, []byte(script), 0o600)) + + // Expect ShellCheck for "sh" to be happy. + // - https://www.shellcheck.net/wiki/SC2148 + cmd := exec.Command(shellcheck, "--enable=all", "--shell=sh", file) + output, err := cmd.CombinedOutput() + assert.NilError(t, err, "%q\n%s", cmd.Args, output) + }) + }) + + t.Run("Long", func(t *testing.T) { + script := MakeDirectories(0o700, "/", strings.Repeat("/asdf", 20)) + + t.Run("PrettyYAML", func(t *testing.T) { + b, err := yaml.Marshal(script) + s := string(b) + assert.NilError(t, err) + assert.Assert(t, !strings.HasPrefix(s, `"`) && !strings.HasPrefix(s, `'`), + "expected plain unquoted scalar, got:\n%s", b) + }) + }) +} From dd8c3202454fe9eee49fa6b8b55d4cc0dfc220f6 Mon Sep 17 00:00:00 2001 From: Chris Bandy Date: Thu, 6 Feb 2025 11:08:43 -0600 Subject: [PATCH 3/5] Store pgAdmin log file positions in the logs directory This prevents log records from being emitted multiple times. --- internal/collector/pgadmin.go | 27 +++-- internal/collector/pgadmin_test.go | 71 +++++++------- internal/controller/standalone_pgadmin/pod.go | 98 ++++++++----------- .../controller/standalone_pgadmin/pod_test.go | 32 ++---- .../standalone_pgadmin/statefulset.go | 12 --- 5 files changed, 99 insertions(+), 141 deletions(-) diff --git a/internal/collector/pgadmin.go b/internal/collector/pgadmin.go index eaa9fc47f5..b108b3997e 100644 --- a/internal/collector/pgadmin.go +++ b/internal/collector/pgadmin.go @@ -21,24 +21,20 @@ func EnablePgAdminLogging(ctx context.Context, spec *v1beta1.InstrumentationSpec return nil } otelConfig := NewConfig(spec) - otelConfig.Extensions["file_storage/pgadmin"] = map[string]any{ - "directory": "/var/log/pgadmin/receiver", - "create_directory": true, - "fsync": true, - } - otelConfig.Extensions["file_storage/gunicorn"] = map[string]any{ - "directory": "/var/log/gunicorn" + "/receiver", - "create_directory": true, + + otelConfig.Extensions["file_storage/pgadmin_data_logs"] = map[string]any{ + "directory": "/var/lib/pgadmin/logs/receiver", + "create_directory": false, "fsync": true, } otelConfig.Receivers["filelog/pgadmin"] = map[string]any{ "include": []string{"/var/lib/pgadmin/logs/pgadmin.log"}, - "storage": "file_storage/pgadmin", + "storage": "file_storage/pgadmin_data_logs", } otelConfig.Receivers["filelog/gunicorn"] = map[string]any{ "include": []string{"/var/lib/pgadmin/logs/gunicorn.log"}, - "storage": "file_storage/gunicorn", + "storage": "file_storage/pgadmin_data_logs", } otelConfig.Processors["resource/pgadmin"] = map[string]any{ @@ -101,7 +97,7 @@ func EnablePgAdminLogging(ctx context.Context, spec *v1beta1.InstrumentationSpec } otelConfig.Pipelines["logs/pgadmin"] = Pipeline{ - Extensions: []ComponentID{"file_storage/pgadmin"}, + Extensions: []ComponentID{"file_storage/pgadmin_data_logs"}, Receivers: []ComponentID{"filelog/pgadmin"}, Processors: []ComponentID{ "resource/pgadmin", @@ -113,7 +109,7 @@ func EnablePgAdminLogging(ctx context.Context, spec *v1beta1.InstrumentationSpec } otelConfig.Pipelines["logs/gunicorn"] = Pipeline{ - Extensions: []ComponentID{"file_storage/gunicorn"}, + Extensions: []ComponentID{"file_storage/pgadmin_data_logs"}, Receivers: []ComponentID{"filelog/gunicorn"}, Processors: []ComponentID{ "resource/pgadmin", @@ -125,9 +121,8 @@ func EnablePgAdminLogging(ctx context.Context, spec *v1beta1.InstrumentationSpec } otelYAML, err := otelConfig.ToYAML() - if err != nil { - return err + if err == nil { + configmap.Data["collector.yaml"] = otelYAML } - configmap.Data["collector.yaml"] = otelYAML - return nil + return err } diff --git a/internal/collector/pgadmin_test.go b/internal/collector/pgadmin_test.go index a05b8c13c2..bca13d7b75 100644 --- a/internal/collector/pgadmin_test.go +++ b/internal/collector/pgadmin_test.go @@ -2,7 +2,7 @@ // // SPDX-License-Identifier: Apache-2.0 -package collector +package collector_test import ( "context" @@ -10,10 +10,12 @@ import ( "gotest.tools/v3/assert" corev1 "k8s.io/api/core/v1" + "sigs.k8s.io/yaml" + "github.com/crunchydata/postgres-operator/internal/collector" + pgadmin "github.com/crunchydata/postgres-operator/internal/controller/standalone_pgadmin" "github.com/crunchydata/postgres-operator/internal/feature" "github.com/crunchydata/postgres-operator/internal/initialize" - "github.com/crunchydata/postgres-operator/internal/naming" "github.com/crunchydata/postgres-operator/internal/testing/cmp" "github.com/crunchydata/postgres-operator/pkg/apis/postgres-operator.crunchydata.com/v1beta1" ) @@ -27,10 +29,9 @@ func TestEnablePgAdminLogging(t *testing.T) { ctx := feature.NewContext(context.Background(), gate) - pgadmin := new(v1beta1.PGAdmin) - configmap := &corev1.ConfigMap{ObjectMeta: naming.StandalonePGAdmin(pgadmin)} + configmap := new(corev1.ConfigMap) initialize.Map(&configmap.Data) - err := EnablePgAdminLogging(ctx, pgadmin.Spec.Instrumentation, configmap) + err := collector.EnablePgAdminLogging(ctx, nil, configmap) assert.NilError(t, err) assert.Assert(t, cmp.MarshalMatches(configmap.Data, ` @@ -41,13 +42,9 @@ collector.yaml: | debug: verbosity: detailed extensions: - file_storage/gunicorn: - create_directory: true - directory: /var/log/gunicorn/receiver - fsync: true - file_storage/pgadmin: - create_directory: true - directory: /var/log/pgadmin/receiver + file_storage/pgadmin_data_logs: + create_directory: false + directory: `+pgadmin.LogDirectoryAbsolutePath+`/receiver fsync: true processors: batch/1s: @@ -86,16 +83,15 @@ collector.yaml: | receivers: filelog/gunicorn: include: - - /var/lib/pgadmin/logs/gunicorn.log - storage: file_storage/gunicorn + - `+pgadmin.GunicornLogFileAbsolutePath+` + storage: file_storage/pgadmin_data_logs filelog/pgadmin: include: - - /var/lib/pgadmin/logs/pgadmin.log - storage: file_storage/pgadmin + - `+pgadmin.LogFileAbsolutePath+` + storage: file_storage/pgadmin_data_logs service: extensions: - - file_storage/gunicorn - - file_storage/pgadmin + - file_storage/pgadmin_data_logs pipelines: logs/gunicorn: exporters: @@ -128,12 +124,22 @@ collector.yaml: | ctx := feature.NewContext(context.Background(), gate) - pgadmin := new(v1beta1.PGAdmin) - pgadmin.Spec.Instrumentation = testInstrumentationSpec() + var spec v1beta1.InstrumentationSpec + assert.NilError(t, yaml.Unmarshal([]byte(`{ + config: { + exporters: { + googlecloud: { + log: { default_log_name: opentelemetry.io/collector-exported-log }, + project: google-project-name, + }, + }, + }, + logs: { exporters: [googlecloud] }, + }`), &spec)) - configmap := &corev1.ConfigMap{ObjectMeta: naming.StandalonePGAdmin(pgadmin)} + configmap := new(corev1.ConfigMap) initialize.Map(&configmap.Data) - err := EnablePgAdminLogging(ctx, pgadmin.Spec.Instrumentation, configmap) + err := collector.EnablePgAdminLogging(ctx, &spec, configmap) assert.NilError(t, err) assert.Assert(t, cmp.MarshalMatches(configmap.Data, ` @@ -148,13 +154,9 @@ collector.yaml: | default_log_name: opentelemetry.io/collector-exported-log project: google-project-name extensions: - file_storage/gunicorn: - create_directory: true - directory: /var/log/gunicorn/receiver - fsync: true - file_storage/pgadmin: - create_directory: true - directory: /var/log/pgadmin/receiver + file_storage/pgadmin_data_logs: + create_directory: false + directory: `+pgadmin.LogDirectoryAbsolutePath+`/receiver fsync: true processors: batch/1s: @@ -193,16 +195,15 @@ collector.yaml: | receivers: filelog/gunicorn: include: - - /var/lib/pgadmin/logs/gunicorn.log - storage: file_storage/gunicorn + - `+pgadmin.GunicornLogFileAbsolutePath+` + storage: file_storage/pgadmin_data_logs filelog/pgadmin: include: - - /var/lib/pgadmin/logs/pgadmin.log - storage: file_storage/pgadmin + - `+pgadmin.LogFileAbsolutePath+` + storage: file_storage/pgadmin_data_logs service: extensions: - - file_storage/gunicorn - - file_storage/pgadmin + - file_storage/pgadmin_data_logs pipelines: logs/gunicorn: exporters: diff --git a/internal/controller/standalone_pgadmin/pod.go b/internal/controller/standalone_pgadmin/pod.go index 3714b46cbd..df70df9132 100644 --- a/internal/controller/standalone_pgadmin/pod.go +++ b/internal/controller/standalone_pgadmin/pod.go @@ -7,6 +7,7 @@ package standalone_pgadmin import ( "context" "fmt" + "path" "strings" corev1 "k8s.io/api/core/v1" @@ -17,6 +18,7 @@ import ( "github.com/crunchydata/postgres-operator/internal/initialize" "github.com/crunchydata/postgres-operator/internal/kubernetes" "github.com/crunchydata/postgres-operator/internal/naming" + "github.com/crunchydata/postgres-operator/internal/shell" "github.com/crunchydata/postgres-operator/pkg/apis/postgres-operator.crunchydata.com/v1beta1" ) @@ -28,8 +30,17 @@ const ( ldapFilePath = "~postgres-operator/ldap-bind-password" gunicornConfigFilePath = "~postgres-operator/" + gunicornConfigKey - // Nothing should be mounted to this location except the script our initContainer writes + // scriptMountPath is where to mount a temporary directory that is only + // writable during Pod initialization. + // + // NOTE: No ConfigMap nor Secret should ever be mounted here because they + // could be used to inject code through "config_system.py". scriptMountPath = "/etc/pgadmin" + + dataMountPath = "/var/lib/pgadmin" + LogDirectoryAbsolutePath = dataMountPath + "/logs" + GunicornLogFileAbsolutePath = LogDirectoryAbsolutePath + "/gunicorn.log" + LogFileAbsolutePath = LogDirectoryAbsolutePath + "/pgadmin.log" ) // pod populates a PodSpec with the container and volumes needed to run pgAdmin. @@ -39,20 +50,10 @@ func pod( outPod *corev1.PodSpec, pgAdminVolume *corev1.PersistentVolumeClaim, ) { - const ( - // config and data volume names - configVolumeName = "pgadmin-config" - dataVolumeName = "pgadmin-data" - pgAdminLogVolumeName = "pgadmin-log" - gunicornLogVolumeName = "gunicorn-log" - scriptVolumeName = "pgadmin-config-system" - tempVolumeName = "tmp" - ) - // create the projected volume of config maps for use in // 1. dynamic server discovery // 2. adding the config variables during pgAdmin startup - configVolume := corev1.Volume{Name: configVolumeName} + configVolume := corev1.Volume{Name: "pgadmin-config"} configVolume.VolumeSource = corev1.VolumeSource{ Projected: &corev1.ProjectedVolumeSource{ Sources: podConfigFiles(inConfigMap, *inPGAdmin), @@ -60,7 +61,7 @@ func pod( } // create the data volume for the persistent database - dataVolume := corev1.Volume{Name: dataVolumeName} + dataVolume := corev1.Volume{Name: "pgadmin-data"} dataVolume.VolumeSource = corev1.VolumeSource{ PersistentVolumeClaim: &corev1.PersistentVolumeClaimVolumeSource{ ClaimName: pgAdminVolume.Name, @@ -68,25 +69,9 @@ func pod( }, } - // create the temp volume for logs - pgAdminLogVolume := corev1.Volume{Name: pgAdminLogVolumeName} - pgAdminLogVolume.VolumeSource = corev1.VolumeSource{ - EmptyDir: &corev1.EmptyDirVolumeSource{ - Medium: corev1.StorageMediumMemory, - }, - } - - // create the temp volume for gunicorn logs - gunicornLogVolume := corev1.Volume{Name: gunicornLogVolumeName} - gunicornLogVolume.VolumeSource = corev1.VolumeSource{ - EmptyDir: &corev1.EmptyDirVolumeSource{ - Medium: corev1.StorageMediumMemory, - }, - } - // Volume used to write a custom config_system.py file in the initContainer // which then loads the configs found in the `configVolume` - scriptVolume := corev1.Volume{Name: scriptVolumeName} + scriptVolume := corev1.Volume{Name: "pgadmin-config-system"} scriptVolume.VolumeSource = corev1.VolumeSource{ EmptyDir: &corev1.EmptyDirVolumeSource{ Medium: corev1.StorageMediumMemory, @@ -101,7 +86,7 @@ func pod( // create a temp volume for restart pid/other/debugging use // TODO: discuss tmp vol vs. persistent vol - tmpVolume := corev1.Volume{Name: tempVolumeName} + tmpVolume := corev1.Volume{Name: "tmp"} tmpVolume.VolumeSource = corev1.VolumeSource{ EmptyDir: &corev1.EmptyDirVolumeSource{ Medium: corev1.StorageMediumMemory, @@ -142,29 +127,21 @@ func pod( }, VolumeMounts: []corev1.VolumeMount{ { - Name: configVolumeName, + Name: configVolume.Name, MountPath: configMountPath, ReadOnly: true, }, { - Name: dataVolumeName, - MountPath: "/var/lib/pgadmin", - }, - { - Name: gunicornLogVolumeName, - MountPath: "/var/log/gunicorn", - }, - { - Name: pgAdminLogVolumeName, - MountPath: "/var/log/pgadmin", + Name: dataVolume.Name, + MountPath: dataMountPath, }, { - Name: scriptVolumeName, + Name: scriptVolume.Name, MountPath: scriptMountPath, ReadOnly: true, }, { - Name: tempVolumeName, + Name: tmpVolume.Name, MountPath: "/tmp", }, }, @@ -199,10 +176,14 @@ func pod( VolumeMounts: []corev1.VolumeMount{ // Volume to write a custom `config_system.py` file to. { - Name: scriptVolumeName, + Name: scriptVolume.Name, MountPath: scriptMountPath, ReadOnly: false, }, + { + Name: dataVolume.Name, + MountPath: dataMountPath, + }, }, } @@ -210,8 +191,6 @@ func pod( outPod.Volumes = []corev1.Volume{ configVolume, dataVolume, - pgAdminLogVolume, - gunicornLogVolume, scriptVolume, tmpVolume, } @@ -426,8 +405,8 @@ if os.path.isfile('` + configDatabaseURIPathAbsolutePath + `'): with open('` + configDatabaseURIPathAbsolutePath + `') as _f: CONFIG_DATABASE_URI = _f.read() -DATA_DIR = '/var/lib/pgadmin' -LOG_FILE = '/var/lib/pgadmin/logs/pgadmin.log' +DATA_DIR = '` + dataMountPath + `' +LOG_FILE = '` + LogFileAbsolutePath + `' LOG_ROTATION_AGE = 24 * 60 # minutes LOG_ROTATION_SIZE = 5 # MiB LOG_ROTATION_MAX_LOG_FILES = 1 @@ -437,18 +416,18 @@ CONSOLE_LOG_LEVEL = logging.WARNING FILE_LOG_LEVEL = logging.INFO FILE_LOG_FORMAT_JSON = {'time': 'created', 'name': 'name', 'level': 'levelname', 'message': 'message'} ` - // gunicorn reads from the `/etc/pgadmin/gunicorn_config.py` file during startup + // Gunicorn reads from the `/etc/pgadmin/gunicorn_config.py` file during startup // after all other config files. // - https://docs.gunicorn.org/en/latest/configure.html#configuration-file // // This command writes a script in `/etc/pgadmin/gunicorn_config.py` that reads // from the `gunicorn-config.json` file and sets those variables globally. - // That way those values are available as settings when gunicorn starts. + // That way those values are available as settings when Gunicorn starts. // - // Note: All gunicorn settings are lowercase with underscores, so ignore + // Note: All Gunicorn settings are lowercase with underscores, so ignore // any keys/names that are not. // - // gunicorn uses the Python logging package, which sets the following attributes: + // Gunicorn uses the Python logging package, which sets the following attributes: // https://docs.python.org/3/library/logging.html#logrecord-attributes. // JsonFormatter is used to format the log: https://pypi.org/project/jsonformatter/ gunicornConfig = ` @@ -457,13 +436,14 @@ with open('` + configMountPath + `/` + gunicornConfigFilePath + `') as _f: _conf, _data = re.compile(r'[a-z_]+'), json.load(_f) if type(_data) is dict: globals().update({k: v for k, v in _data.items() if _conf.fullmatch(k)}) + gunicorn.SERVER_SOFTWARE = 'Python' logconfig_dict = copy.deepcopy(gunicorn.glogging.CONFIG_DEFAULTS) logconfig_dict['loggers']['gunicorn.access']['handlers'] = ['file'] logconfig_dict['loggers']['gunicorn.error']['handlers'] = ['file'] logconfig_dict['handlers']['file'] = { 'class': 'logging.handlers.RotatingFileHandler', - 'filename': '/var/lib/pgadmin/logs/gunicorn.log', + 'filename': '` + GunicornLogFileAbsolutePath + `', 'backupCount': 1, 'maxBytes': 2 << 20, # MiB 'formatter': 'json', } @@ -483,9 +463,15 @@ logconfig_dict['formatters']['json'] = { args := []string{strings.TrimLeft(configSystem, "\n"), strings.TrimLeft(gunicornConfig, "\n")} script := strings.Join([]string{ - // Use the initContainer to create this path to avoid the error noted here: + // Create the config directory so Kubernetes can mount it later. // - https://issue.k8s.io/121294 - `mkdir -p ` + configMountPath, + shell.MakeDirectories(0o775, scriptMountPath, configMountPath), + + // Create the logs directory with g+rwx so the OTel Collector can + // write to it as well. + // TODO(log-rotation): Move the last segment into the Collector startup. + shell.MakeDirectories(0o775, dataMountPath, path.Join(LogDirectoryAbsolutePath, "receiver")), + // Write the system and server configurations. `echo "$1" > ` + scriptMountPath + `/config_system.py`, `echo "$2" > ` + scriptMountPath + `/gunicorn_config.py`, diff --git a/internal/controller/standalone_pgadmin/pod_test.go b/internal/controller/standalone_pgadmin/pod_test.go index e51dbd4fe8..ce3ad076d2 100644 --- a/internal/controller/standalone_pgadmin/pod_test.go +++ b/internal/controller/standalone_pgadmin/pod_test.go @@ -127,10 +127,6 @@ containers: readOnly: true - mountPath: /var/lib/pgadmin name: pgadmin-data - - mountPath: /var/log/gunicorn - name: gunicorn-log - - mountPath: /var/log/pgadmin - name: pgadmin-log - mountPath: /etc/pgadmin name: pgadmin-config-system readOnly: true @@ -142,7 +138,8 @@ initContainers: - -ceu - -- - |- - mkdir -p /etc/pgadmin/conf.d + mkdir -p '/etc/pgadmin/conf.d' && chmod 0775 '/etc/pgadmin/conf.d' + mkdir -p '/var/lib/pgadmin/logs/receiver' && chmod 0775 '/var/lib/pgadmin/logs/receiver' '/var/lib/pgadmin/logs' echo "$1" > /etc/pgadmin/config_system.py echo "$2" > /etc/pgadmin/gunicorn_config.py - startup @@ -176,6 +173,7 @@ initContainers: _conf, _data = re.compile(r'[a-z_]+'), json.load(_f) if type(_data) is dict: globals().update({k: v for k, v in _data.items() if _conf.fullmatch(k)}) + gunicorn.SERVER_SOFTWARE = 'Python' logconfig_dict = copy.deepcopy(gunicorn.glogging.CONFIG_DEFAULTS) logconfig_dict['loggers']['gunicorn.access']['handlers'] = ['file'] @@ -211,6 +209,8 @@ initContainers: volumeMounts: - mountPath: /etc/pgadmin name: pgadmin-config-system + - mountPath: /var/lib/pgadmin + name: pgadmin-data volumes: - name: pgadmin-config projected: @@ -226,12 +226,6 @@ volumes: - name: pgadmin-data persistentVolumeClaim: claimName: "" -- emptyDir: - medium: Memory - name: pgadmin-log -- emptyDir: - medium: Memory - name: gunicorn-log - emptyDir: medium: Memory sizeLimit: 32Ki @@ -352,10 +346,6 @@ containers: readOnly: true - mountPath: /var/lib/pgadmin name: pgadmin-data - - mountPath: /var/log/gunicorn - name: gunicorn-log - - mountPath: /var/log/pgadmin - name: pgadmin-log - mountPath: /etc/pgadmin name: pgadmin-config-system readOnly: true @@ -367,7 +357,8 @@ initContainers: - -ceu - -- - |- - mkdir -p /etc/pgadmin/conf.d + mkdir -p '/etc/pgadmin/conf.d' && chmod 0775 '/etc/pgadmin/conf.d' + mkdir -p '/var/lib/pgadmin/logs/receiver' && chmod 0775 '/var/lib/pgadmin/logs/receiver' '/var/lib/pgadmin/logs' echo "$1" > /etc/pgadmin/config_system.py echo "$2" > /etc/pgadmin/gunicorn_config.py - startup @@ -401,6 +392,7 @@ initContainers: _conf, _data = re.compile(r'[a-z_]+'), json.load(_f) if type(_data) is dict: globals().update({k: v for k, v in _data.items() if _conf.fullmatch(k)}) + gunicorn.SERVER_SOFTWARE = 'Python' logconfig_dict = copy.deepcopy(gunicorn.glogging.CONFIG_DEFAULTS) logconfig_dict['loggers']['gunicorn.access']['handlers'] = ['file'] @@ -440,6 +432,8 @@ initContainers: volumeMounts: - mountPath: /etc/pgadmin name: pgadmin-config-system + - mountPath: /var/lib/pgadmin + name: pgadmin-data volumes: - name: pgadmin-config projected: @@ -455,12 +449,6 @@ volumes: - name: pgadmin-data persistentVolumeClaim: claimName: "" -- emptyDir: - medium: Memory - name: pgadmin-log -- emptyDir: - medium: Memory - name: gunicorn-log - emptyDir: medium: Memory sizeLimit: 32Ki diff --git a/internal/controller/standalone_pgadmin/statefulset.go b/internal/controller/standalone_pgadmin/statefulset.go index f3e5712614..fc47cea99c 100644 --- a/internal/controller/standalone_pgadmin/statefulset.go +++ b/internal/controller/standalone_pgadmin/statefulset.go @@ -122,24 +122,12 @@ func statefulset( pod(pgadmin, configmap, &sts.Spec.Template.Spec, dataVolume) if feature.Enabled(ctx, feature.OpenTelemetryLogs) { - // Mount for file_storage/pgadmin - pgAdminLogVolumeMount := corev1.VolumeMount{ - Name: "pgadmin-log", - MountPath: "/var/log/pgadmin", - } - // Mount for file_storage/gunicorn - gunicornLogVolumeMount := corev1.VolumeMount{ - Name: "gunicorn-log", - MountPath: "/var/log/gunicorn", - } // Logs for gunicorn and pgadmin write to /var/lib/pgadmin/logs dataVolumeMount := corev1.VolumeMount{ Name: "pgadmin-data", MountPath: "/var/lib/pgadmin", } volumeMounts := []corev1.VolumeMount{ - pgAdminLogVolumeMount, - gunicornLogVolumeMount, dataVolumeMount, } From e78b4efbd3f16dbb8fd3510461cecbd58a387df7 Mon Sep 17 00:00:00 2001 From: Chris Bandy Date: Mon, 10 Feb 2025 13:51:06 -0600 Subject: [PATCH 4/5] Ensure Postgres and Patroni log directories are writable The `install` command only sets permissions on the final directory. --- internal/collector/patroni.go | 1 + internal/collector/pgbackrest.go | 1 + internal/collector/pgbouncer.go | 1 + internal/collector/postgres.go | 2 ++ internal/postgres/config.go | 22 +++++++++------------- internal/postgres/reconcile_test.go | 20 ++++++++------------ 6 files changed, 22 insertions(+), 25 deletions(-) diff --git a/internal/collector/patroni.go b/internal/collector/patroni.go index d44e1744cd..3199d9c0ea 100644 --- a/internal/collector/patroni.go +++ b/internal/collector/patroni.go @@ -21,6 +21,7 @@ func EnablePatroniLogging(ctx context.Context, // Keep track of what log records and files have been processed. // Use a subdirectory of the logs directory to stay within the same failure domain. + // TODO(log-rotation): Create this directory during Collector startup. // // https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/-/extension/storage/filestorage#readme outConfig.Extensions["file_storage/patroni_logs"] = map[string]any{ diff --git a/internal/collector/pgbackrest.go b/internal/collector/pgbackrest.go index 33fb2e0922..569829bf0e 100644 --- a/internal/collector/pgbackrest.go +++ b/internal/collector/pgbackrest.go @@ -47,6 +47,7 @@ func NewConfigForPgBackrestRepoHostPod( // Keep track of what log records and files have been processed. // Use a subdirectory of the logs directory to stay within the same failure domain. + // TODO(log-rotation): Create this directory during Collector startup. config.Extensions["file_storage/pgbackrest_logs"] = map[string]any{ "directory": directory + "/receiver", "create_directory": true, diff --git a/internal/collector/pgbouncer.go b/internal/collector/pgbouncer.go index 23ae429d95..4281399e3e 100644 --- a/internal/collector/pgbouncer.go +++ b/internal/collector/pgbouncer.go @@ -50,6 +50,7 @@ func EnablePgBouncerLogging(ctx context.Context, // Keep track of what log records and files have been processed. // Use a subdirectory of the logs directory to stay within the same failure domain. + // TODO(log-rotation): Create this directory during Collector startup. // // https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/-/extension/storage/filestorage#readme outConfig.Extensions["file_storage/pgbouncer_logs"] = map[string]any{ diff --git a/internal/collector/postgres.go b/internal/collector/postgres.go index cbf37c46a9..544f0e9feb 100644 --- a/internal/collector/postgres.go +++ b/internal/collector/postgres.go @@ -110,6 +110,7 @@ func EnablePostgresLogging( // Keep track of what log records and files have been processed. // Use a subdirectory of the logs directory to stay within the same failure domain. + // TODO(log-rotation): Create this directory during Collector startup. // // https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/-/extension/storage/filestorage#readme outConfig.Extensions["file_storage/postgres_logs"] = map[string]any{ @@ -215,6 +216,7 @@ func EnablePostgresLogging( } // pgBackRest pipeline + // TODO(log-rotation): Create this directory during Collector startup. outConfig.Extensions["file_storage/pgbackrest_logs"] = map[string]any{ "directory": naming.PGBackRestPGDataLogPath + "/receiver", "create_directory": true, diff --git a/internal/postgres/config.go b/internal/postgres/config.go index 7b265fa362..8c3705f814 100644 --- a/internal/postgres/config.go +++ b/internal/postgres/config.go @@ -14,6 +14,7 @@ import ( "github.com/crunchydata/postgres-operator/internal/config" "github.com/crunchydata/postgres-operator/internal/feature" "github.com/crunchydata/postgres-operator/internal/naming" + "github.com/crunchydata/postgres-operator/internal/shell" "github.com/crunchydata/postgres-operator/pkg/apis/postgres-operator.crunchydata.com/v1beta1" ) @@ -297,9 +298,9 @@ chmod +x /tmp/pg_rewind_tde.sh ` } - args := []string{version, walDir, naming.PGBackRestPGDataLogPath, naming.PatroniPGDataLogPath} + args := []string{version, walDir} script := strings.Join([]string{ - `declare -r expected_major_version="$1" pgwal_directory="$2" pgbrLog_directory="$3" patroniLog_directory="$4"`, + `declare -r expected_major_version="$1" pgwal_directory="$2"`, // Function to print the permissions of a file or directory and its parents. bashPermissions, @@ -370,17 +371,12 @@ chmod +x /tmp/pg_rewind_tde.sh `else (halt Permissions!); fi ||`, `halt "$(permissions "${postgres_data_directory}" ||:)"`, - // Create the pgBackRest log directory. - `results 'pgBackRest log directory' "${pgbrLog_directory}"`, - `install --directory --mode=0775 "${pgbrLog_directory}" ||`, - `halt "$(permissions "${pgbrLog_directory}" ||:)"`, - - // Create the Patroni log directory. - `results 'Patroni log directory' "${patroniLog_directory}"`, - `install --directory --mode=0775 "${patroniLog_directory}" ||`, - `halt "$(permissions "${patroniLog_directory}" ||:)"`, - - `install --directory --mode=0775 ` + LogDirectory() + ` ||`, + // Create log directories. + `(` + shell.MakeDirectories(0o775, dataMountPath, naming.PGBackRestPGDataLogPath) + `) ||`, + `halt "$(permissions ` + naming.PGBackRestPGDataLogPath + ` ||:)"`, + `(` + shell.MakeDirectories(0o775, dataMountPath, naming.PatroniPGDataLogPath) + `) ||`, + `halt "$(permissions ` + naming.PatroniPGDataLogPath + ` ||:)"`, + `(` + shell.MakeDirectories(0o775, dataMountPath, LogDirectory()) + `) ||`, `halt "$(permissions ` + LogDirectory() + ` ||:)"`, // Copy replication client certificate files diff --git a/internal/postgres/reconcile_test.go b/internal/postgres/reconcile_test.go index d7ccb3b773..3898f28512 100644 --- a/internal/postgres/reconcile_test.go +++ b/internal/postgres/reconcile_test.go @@ -230,7 +230,7 @@ initContainers: - -ceu - -- - |- - declare -r expected_major_version="$1" pgwal_directory="$2" pgbrLog_directory="$3" patroniLog_directory="$4" + declare -r expected_major_version="$1" pgwal_directory="$2" permissions() { while [[ -n "$1" ]]; do set "${1%/*}" "$@"; done; shift; stat -Lc '%A %4u %4g %n' "$@"; } halt() { local rc=$?; >&2 echo "$@"; exit "${rc/#0/1}"; } results() { printf '::postgres-operator: %s::%s\n' "$@"; } @@ -267,13 +267,11 @@ initContainers: recreate "${postgres_data_directory}" '0700' else (halt Permissions!); fi || halt "$(permissions "${postgres_data_directory}" ||:)" - results 'pgBackRest log directory' "${pgbrLog_directory}" - install --directory --mode=0775 "${pgbrLog_directory}" || - halt "$(permissions "${pgbrLog_directory}" ||:)" - results 'Patroni log directory' "${patroniLog_directory}" - install --directory --mode=0775 "${patroniLog_directory}" || - halt "$(permissions "${patroniLog_directory}" ||:)" - install --directory --mode=0775 /pgdata/logs/postgres || + (mkdir -p '/pgdata/pgbackrest/log' && chmod 0775 '/pgdata/pgbackrest/log' '/pgdata/pgbackrest') || + halt "$(permissions /pgdata/pgbackrest/log ||:)" + (mkdir -p '/pgdata/patroni/log' && chmod 0775 '/pgdata/patroni/log' '/pgdata/patroni') || + halt "$(permissions /pgdata/patroni/log ||:)" + (mkdir -p '/pgdata/logs/postgres' && chmod 0775 '/pgdata/logs/postgres' '/pgdata/logs') || halt "$(permissions /pgdata/logs/postgres ||:)" install -D --mode=0600 -t "/tmp/replication" "/pgconf/tls/replication"/{tls.crt,tls.key,ca.crt} @@ -290,8 +288,6 @@ initContainers: - startup - "11" - /pgdata/pg11_wal - - /pgdata/pgbackrest/log - - /pgdata/patroni/log env: - name: PGDATA value: /pgdata/pg11 @@ -479,7 +475,7 @@ volumes: // Startup moves WAL files to data volume. assert.DeepEqual(t, pod.InitContainers[0].Command[4:], - []string{"startup", "11", "/pgdata/pg11_wal", "/pgdata/pgbackrest/log", "/pgdata/patroni/log"}) + []string{"startup", "11", "/pgdata/pg11_wal"}) }) t.Run("WithAdditionalConfigFiles", func(t *testing.T) { @@ -709,7 +705,7 @@ volumes: // Startup moves WAL files to WAL volume. assert.DeepEqual(t, pod.InitContainers[0].Command[4:], - []string{"startup", "11", "/pgwal/pg11_wal", "/pgdata/pgbackrest/log", "/pgdata/patroni/log"}) + []string{"startup", "11", "/pgwal/pg11_wal"}) }) } From 7cdd56f9b1b83c9234501e5eec0df2e36d47af62 Mon Sep 17 00:00:00 2001 From: Chris Bandy Date: Tue, 11 Feb 2025 13:18:20 -0600 Subject: [PATCH 5/5] Ensure pgBackRest log directories are writable The `install` command only sets permissions on the final directory. --- internal/pgbackrest/config.go | 6 +++++- internal/pgbackrest/config_test.go | 2 +- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/internal/pgbackrest/config.go b/internal/pgbackrest/config.go index 873d1cbf8b..bfbf6f8d63 100644 --- a/internal/pgbackrest/config.go +++ b/internal/pgbackrest/config.go @@ -7,6 +7,7 @@ package pgbackrest import ( "context" "fmt" + "path" "strconv" "strings" "time" @@ -19,6 +20,7 @@ import ( "github.com/crunchydata/postgres-operator/internal/initialize" "github.com/crunchydata/postgres-operator/internal/naming" "github.com/crunchydata/postgres-operator/internal/postgres" + "github.com/crunchydata/postgres-operator/internal/shell" "github.com/crunchydata/postgres-operator/pkg/apis/postgres-operator.crunchydata.com/v1beta1" ) @@ -153,7 +155,9 @@ func MakePGBackrestLogDir(template *corev1.PodTemplateSpec, } container := corev1.Container{ - Command: []string{"bash", "-c", "umask 000 && install -m 777 -d " + pgBackRestLogPath}, + // TODO(log-rotation): The second argument here should be the path + // of the volume mount. Find a way to calculate that consistently. + Command: []string{"bash", "-c", shell.MakeDirectories(0o775, path.Dir(pgBackRestLogPath), pgBackRestLogPath)}, Image: config.PGBackRestContainerImage(cluster), ImagePullPolicy: cluster.Spec.ImagePullPolicy, Name: naming.ContainerPGBackRestLogDirInit, diff --git a/internal/pgbackrest/config_test.go b/internal/pgbackrest/config_test.go index 065bd70495..08aaaf8d94 100644 --- a/internal/pgbackrest/config_test.go +++ b/internal/pgbackrest/config_test.go @@ -292,7 +292,7 @@ func TestMakePGBackrestLogDir(t *testing.T) { for _, c := range podTemplate.Spec.InitContainers { if c.Name == naming.ContainerPGBackRestLogDirInit { // ignore "bash -c", should skip repo with no volume - assert.Equal(t, "umask 000 && install -m 777 -d /pgbackrest/repo2/log", c.Command[2]) + assert.Equal(t, `mkdir -p '/pgbackrest/repo2/log' && chmod 0775 '/pgbackrest/repo2/log'`, c.Command[2]) assert.Equal(t, c.Image, "test-image") assert.Equal(t, c.ImagePullPolicy, corev1.PullAlways) assert.Assert(t, !cmp.DeepEqual(c.SecurityContext,