From 769973efe9ef2e698519dc3867f3747b9d3d9ad9 Mon Sep 17 00:00:00 2001 From: Chris Bandy Date: Mon, 21 Apr 2025 16:38:15 -0500 Subject: [PATCH] Create directories with group-write permissions The group-write permission is important for persistent file systems in environments where different containers are assigned different UIDs over time. Some network file systems, however, reject attempts to set POSIX directory permissions. CIFS and NFS are notable in this regard. Issue: PGO-2417 --- internal/collector/instance.go | 3 +-- internal/controller/standalone_pgadmin/pod.go | 6 ++--- .../controller/standalone_pgadmin/pod_test.go | 8 +++---- internal/pgbackrest/config.go | 2 +- internal/pgbackrest/config_test.go | 2 +- internal/postgres/config.go | 6 ++--- internal/postgres/reconcile_test.go | 6 ++--- internal/shell/paths.go | 22 +++++++++++++------ internal/shell/paths_test.go | 12 +++++----- 9 files changed, 37 insertions(+), 30 deletions(-) diff --git a/internal/collector/instance.go b/internal/collector/instance.go index f37eb7f4c3..8158d9dda3 100644 --- a/internal/collector/instance.go +++ b/internal/collector/instance.go @@ -180,8 +180,7 @@ func startCommand(logDirectories []string, includeLogrotate bool) []string { if len(logDirectories) != 0 { for _, logDir := range logDirectories { mkdirScript = mkdirScript + ` -` + shell.MakeDirectories(0o775, logDir, - path.Join(logDir, "receiver")) +` + shell.MakeDirectories(logDir, path.Join(logDir, "receiver")) } } diff --git a/internal/controller/standalone_pgadmin/pod.go b/internal/controller/standalone_pgadmin/pod.go index 6eab70ec7a..71f785c15e 100644 --- a/internal/controller/standalone_pgadmin/pod.go +++ b/internal/controller/standalone_pgadmin/pod.go @@ -442,10 +442,10 @@ with open('` + configMountPath + `/` + gunicornConfigFilePath + `') as _f: script := strings.Join([]string{ // Create the config directory so Kubernetes can mount it later. // - https://issue.k8s.io/121294 - shell.MakeDirectories(0o775, scriptMountPath, configMountPath), + shell.MakeDirectories(scriptMountPath, configMountPath), - // Create the logs directory with g+rwx to ensure pgAdmin can write to it as well. - shell.MakeDirectories(0o775, dataMountPath, LogDirectoryAbsolutePath), + // Create the logs directory and ensure pgAdmin can write to it as well. + shell.MakeDirectories(dataMountPath, LogDirectoryAbsolutePath), // Write the system and server configurations. `echo "$1" > ` + scriptMountPath + `/config_system.py`, diff --git a/internal/controller/standalone_pgadmin/pod_test.go b/internal/controller/standalone_pgadmin/pod_test.go index 84f6e56cdc..b30b35bc65 100644 --- a/internal/controller/standalone_pgadmin/pod_test.go +++ b/internal/controller/standalone_pgadmin/pod_test.go @@ -137,8 +137,8 @@ initContainers: - -ceu - -- - |- - mkdir -p '/etc/pgadmin/conf.d' && chmod 0775 '/etc/pgadmin/conf.d' - mkdir -p '/var/lib/pgadmin/logs' && chmod 0775 '/var/lib/pgadmin/logs' + mkdir -p '/etc/pgadmin/conf.d' && { chmod 0775 '/etc/pgadmin/conf.d' || :; } + mkdir -p '/var/lib/pgadmin/logs' && { chmod 0775 '/var/lib/pgadmin/logs' || :; } echo "$1" > /etc/pgadmin/config_system.py echo "$2" > /etc/pgadmin/gunicorn_config.py - startup @@ -342,8 +342,8 @@ initContainers: - -ceu - -- - |- - mkdir -p '/etc/pgadmin/conf.d' && chmod 0775 '/etc/pgadmin/conf.d' - mkdir -p '/var/lib/pgadmin/logs' && chmod 0775 '/var/lib/pgadmin/logs' + mkdir -p '/etc/pgadmin/conf.d' && { chmod 0775 '/etc/pgadmin/conf.d' || :; } + mkdir -p '/var/lib/pgadmin/logs' && { chmod 0775 '/var/lib/pgadmin/logs' || :; } echo "$1" > /etc/pgadmin/config_system.py echo "$2" > /etc/pgadmin/gunicorn_config.py - startup diff --git a/internal/pgbackrest/config.go b/internal/pgbackrest/config.go index 498be32d3b..c99e952afc 100644 --- a/internal/pgbackrest/config.go +++ b/internal/pgbackrest/config.go @@ -177,7 +177,7 @@ func MakePGBackrestLogDir(template *corev1.PodTemplateSpec, container := corev1.Container{ // 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)}, + Command: []string{"bash", "-c", shell.MakeDirectories(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 08aaaf8d94..a314ad3102 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, `mkdir -p '/pgbackrest/repo2/log' && chmod 0775 '/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, diff --git a/internal/postgres/config.go b/internal/postgres/config.go index a478c0e72b..9270472163 100644 --- a/internal/postgres/config.go +++ b/internal/postgres/config.go @@ -375,11 +375,11 @@ chmod +x /tmp/pg_rewind_tde.sh `halt "$(permissions "${postgres_data_directory}" ||:)"`, // Create log directories. - `(` + shell.MakeDirectories(0o775, dataMountPath, naming.PGBackRestPGDataLogPath) + `) ||`, + `(` + shell.MakeDirectories(dataMountPath, naming.PGBackRestPGDataLogPath) + `) ||`, `halt "$(permissions ` + naming.PGBackRestPGDataLogPath + ` ||:)"`, - `(` + shell.MakeDirectories(0o775, dataMountPath, naming.PatroniPGDataLogPath) + `) ||`, + `(` + shell.MakeDirectories(dataMountPath, naming.PatroniPGDataLogPath) + `) ||`, `halt "$(permissions ` + naming.PatroniPGDataLogPath + ` ||:)"`, - `(` + shell.MakeDirectories(0o775, dataMountPath, LogDirectory()) + `) ||`, + `(` + shell.MakeDirectories(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 9903afb97c..ba3a90b57b 100644 --- a/internal/postgres/reconcile_test.go +++ b/internal/postgres/reconcile_test.go @@ -268,11 +268,11 @@ initContainers: recreate "${postgres_data_directory}" '0700' else (halt Permissions!); fi || halt "$(permissions "${postgres_data_directory}" ||:)" - (mkdir -p '/pgdata/pgbackrest/log' && chmod 0775 '/pgdata/pgbackrest/log' '/pgdata/pgbackrest') || + (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') || + (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') || + (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} diff --git a/internal/shell/paths.go b/internal/shell/paths.go index d1df635e68..94c997f7b4 100644 --- a/internal/shell/paths.go +++ b/internal/shell/paths.go @@ -33,14 +33,14 @@ func CleanFileName(path string) string { // 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. +// base and sets their permissions for Kubernetes, 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 { +func MakeDirectories(base string, paths ...string) string { // Without any paths, return a command that succeeds when the base path // exists. if len(paths) == 0 { @@ -61,14 +61,22 @@ func MakeDirectories(perms fs.FileMode, base string, paths ...string) string { } } + const perms fs.FileMode = 0 | + // S_IRWXU: enable owner read, write, and execute permissions. + 0o0700 | + // S_IRWXG: enable group read, write, and execute permissions. + 0o0070 | + // S_IXOTH, S_IROTH: enable other read and execute permissions. + 0o0001 | 0o0004 + 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...), " "), + // Try to set the permissions of every path and each parent. + // This swallows the exit status of `chmod` because not all filesystems + // tolerate the operation; CIFS and NFS are notable examples. + fmt.Sprintf(` && { chmod %#o %s || :; }`, + perms, strings.Join(QuoteWords(allPaths...), " "), ) } diff --git a/internal/shell/paths_test.go b/internal/shell/paths_test.go index 8af16a73c0..33e68c2332 100644 --- a/internal/shell/paths_test.go +++ b/internal/shell/paths_test.go @@ -52,20 +52,20 @@ func TestMakeDirectories(t *testing.T) { t.Run("NoPaths", func(t *testing.T) { assert.Equal(t, - MakeDirectories(0o755, "/asdf/jklm"), + MakeDirectories("/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'`) + MakeDirectories("/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") + script := MakeDirectories("/asdf", "/asdf/qwerty/boots") assert.DeepEqual(t, script, - `mkdir -p '/asdf/qwerty/boots' && chmod 0775 '/asdf/qwerty/boots' '/asdf/qwerty'`) + `mkdir -p '/asdf/qwerty/boots' && { chmod 0775 '/asdf/qwerty/boots' '/asdf/qwerty' || :; }`) t.Run("ShellCheckPOSIX", func(t *testing.T) { shellcheck := require.ShellCheck(t) @@ -83,7 +83,7 @@ func TestMakeDirectories(t *testing.T) { }) t.Run("Long", func(t *testing.T) { - script := MakeDirectories(0o700, "/", strings.Repeat("/asdf", 20)) + script := MakeDirectories("/", strings.Repeat("/asdf", 20)) t.Run("PrettyYAML", func(t *testing.T) { b, err := yaml.Marshal(script)