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)