Ensure Postgres data directories have group-read permission

Makefile

@@ -13,7 +13,7 @@ CONTROLLER ?= $(GO) tool sigs.k8s.io/controller-tools/cmd/controller-gen
 
 # Run tests using the latest tools.
 ENVTEST ?= $(GO) run sigs.k8s.io/controller-runtime/tools/setup-envtest@latest
-KUTTL ?= $(GO) run github.com/kudobuilder/kuttl/pkg/kuttlctl/cmd/kubectl-kuttl@latest
+KUTTL ?= $(GO) run github.com/kudobuilder/kuttl/cmd/kubectl-kuttl@latest
 KUTTL_TEST ?= $(KUTTL) test
 
 ##@ General
@@ -171,6 +171,10 @@ check-envtest-existing: createnamespaces
 	kubectl delete -k ./config/dev
 
 # Expects operator to be running
+#
+# KUTTL runs with a single kubectl context named "cluster".
+# If you experience `cluster "minikube" does not exist`, try `MINIKUBE_PROFILE=cluster`.
+#
 .PHONY: check-kuttl
 check-kuttl: ## Run kuttl end-to-end tests
 check-kuttl: ## example command: make check-kuttl KUTTL_TEST='

internal/controller/pgupgrade/jobs.go

@@ -98,7 +98,7 @@ func upgradeCommand(spec *v1beta1.PGUpgradeSettings, fetchKeyCommand string) []s
 		// proper permissions have to be set on the old pgdata directory and the
 		// preload library settings must be copied over.
 		`echo -e "\nStep 3: Setting the expected permissions on the old pgdata directory...\n"`,
-		`chmod 700 /pgdata/pg"${old_version}"`,
+		`chmod 750 /pgdata/pg"${old_version}"`,
 		`echo -e "Step 4: Copying shared_preload_libraries setting to new postgresql.conf file...\n"`,
 		`echo "shared_preload_libraries = '$(/usr/pgsql-"""${old_version}"""/bin/postgres -D \`,
 		`/pgdata/pg"""${old_version}""" -C shared_preload_libraries)'" >> /pgdata/pg"${new_version}"/postgresql.conf`,

internal/controller/pgupgrade/jobs_test.go

@@ -214,7 +214,7 @@ spec:
           echo -e "Step 2: Initializing new pgdata directory...\n"
           /usr/pgsql-"${new_version}"/bin/initdb -k -D /pgdata/pg"${new_version}"
           echo -e "\nStep 3: Setting the expected permissions on the old pgdata directory...\n"
-          chmod 700 /pgdata/pg"${old_version}"
+          chmod 750 /pgdata/pg"${old_version}"
           echo -e "Step 4: Copying shared_preload_libraries setting to new postgresql.conf file...\n"
           echo "shared_preload_libraries = '$(/usr/pgsql-"""${old_version}"""/bin/postgres -D \
           /pgdata/pg"""${old_version}""" -C shared_preload_libraries)'" >> /pgdata/pg"${new_version}"/postgresql.conf

internal/controller/runtime/pod_client.go

@@ -24,8 +24,8 @@ type podExecutor func(
 ) error
 
 func newPodClient(config *rest.Config) (rest.Interface, error) {
-	codecs := serializer.NewCodecFactory(scheme.Scheme)
-	gvk, _ := apiutil.GVKForObject(&corev1.Pod{}, scheme.Scheme)
+	codecs := serializer.NewCodecFactory(Scheme)
+	gvk, _ := apiutil.GVKForObject(&corev1.Pod{}, Scheme)
 	httpClient, err := rest.HTTPClientFor(config)
 	if err != nil {
 		return nil, err

internal/patroni/config.go

@@ -518,22 +518,7 @@ func instanceYAML(
 	if command := pgbackrestReplicaCreateCommand; len(command) > 0 {
 
 		// Regardless of the "keep_data" setting below, Patroni deletes the
-		// data directory when all methods fail. pgBackRest will not restore
-		// when the data directory is missing, so create it before running the
-		// command. PostgreSQL requires that the directory is writable by only
-		// itself.
-		// - https://github.com/zalando/patroni/blob/v2.0.2/patroni/ha.py#L249
-		// - https://github.com/pgbackrest/pgbackrest/issues/1445
-		// - https://git.postgresql.org/gitweb/?p=postgresql.git;f=src/backend/utils/init/miscinit.c;hb=REL_13_0#l319
-		//
-		// NOTE(cbandy): The "PATRONI_POSTGRESQL_DATA_DIR" environment variable
-		// is defined in this package, but it is removed by Patroni at runtime.
-		command = append([]string{
-			"bash", "-ceu", "--",
-			`install --directory --mode=0700 "${PGDATA?}" && exec "$@"`,
-			"-",
-		}, command...)
-
+		// data directory when all methods fail.
 		postgresql[pgBackRestCreateReplicaMethod] = map[string]any{
 			"command":   strings.Join(shell.QuoteWords(command...), " "),
 			"keep_data": true, // Use the data directory from a prior method.

internal/patroni/config_test.go

@@ -720,8 +720,7 @@ postgresql:
   - pgbackrest
   - basebackup
   pgbackrest:
-    command: '''bash'' ''-ceu'' ''--'' ''install --directory --mode=0700 "${PGDATA?}"
-      && exec "$@"'' ''-'' ''some'' ''backrest'' ''cmd'''
+    command: '''some'' ''backrest'' ''cmd'''
     keep_data: true
     no_leader: true
     no_params: true
@@ -786,40 +785,17 @@ func TestPGBackRestCreateReplicaCommand(t *testing.T) {
 	}
 	assert.NilError(t, yaml.Unmarshal([]byte(data), &parsed))
 
-	dir := t.TempDir()
+	assert.Equal(t, parsed.PostgreSQL.PGBackRest.Command, `'some' 'backrest' 'cmd'`)
 
 	// The command should be compatible with any shell.
 	{
-		command := parsed.PostgreSQL.PGBackRest.Command
-		file := filepath.Join(dir, "command.sh")
-		assert.NilError(t, os.WriteFile(file, []byte(command), 0o600))
+		file := filepath.Join(t.TempDir(), "command.sh")
+		assert.NilError(t, os.WriteFile(file, []byte(parsed.PostgreSQL.PGBackRest.Command), 0o600))
 
 		cmd := exec.CommandContext(t.Context(), shellcheck, "--enable=all", "--shell=sh", file)
 		output, err := cmd.CombinedOutput()
 		assert.NilError(t, err, "%q\n%s", cmd.Args, output)
 	}
-
-	// Naive parsing of shell words...
-	command := strings.Split(strings.Trim(parsed.PostgreSQL.PGBackRest.Command, "'"), "' '")
-
-	// Expect a bash command with an inline script.
-	assert.DeepEqual(t, command[:3], []string{"bash", "-ceu", "--"})
-	assert.Assert(t, len(command) > 3)
-	script := command[3]
-
-	// It should call the pgBackRest command.
-	assert.Assert(t, strings.HasSuffix(script, ` exec "$@"`))
-	assert.DeepEqual(t, command[len(command)-3:], []string{"some", "backrest", "cmd"})
-
-	// It should pass shellcheck.
-	{
-		file := filepath.Join(dir, "script.bash")
-		assert.NilError(t, os.WriteFile(file, []byte(script), 0o600))
-
-		cmd := exec.CommandContext(t.Context(), shellcheck, "--enable=all", file)
-		output, err := cmd.CombinedOutput()
-		assert.NilError(t, err, "%q\n%s", cmd.Args, output)
-	}
 }
 
 func TestProbeTiming(t *testing.T) {

internal/pgbackrest/config.go

@@ -356,7 +356,7 @@ func DedicatedSnapshotVolumeRestoreCommand(pgdata string, args ...string) []stri
 BACKUP_LABEL=$([[ ! -e "${pgdata}/backup_label" ]] || md5sum "${pgdata}/backup_label")
 echo "Starting pgBackRest delta restore"
 
-install --directory --mode=0700 "${pgdata}"
+install --directory --mode=0750 "${pgdata}"
 rm -f "${pgdata}/postmaster.pid"
 bash -xc "pgbackrest restore ${opts}"
 rm -f "${pgdata}/patroni.dynamic.json"

internal/postgres/config.go

@@ -341,9 +341,9 @@ func startupCommand(
 		// and so we add the subdirectory `data` in order to set the permissions.
 		checkInstallRecreateCmd := strings.Join([]string{
 			`if [[ ! -e "${tablespace_dir}" || -O "${tablespace_dir}" ]]; then`,
-			`install --directory --mode=0700 "${tablespace_dir}"`,
+			`install --directory --mode=0750 "${tablespace_dir}"`,
 			`elif [[ -w "${tablespace_dir}" && -g "${tablespace_dir}" ]]; then`,
-			`recreate "${tablespace_dir}" '0700'`,
+			`recreate "${tablespace_dir}" '0750'`,
 			`else (halt Permissions!); fi ||`,
 			`halt "$(permissions "${tablespace_dir}" ||:)"`,
 		}, "\n")
@@ -419,23 +419,24 @@ chmod +x /tmp/pg_rewind_tde.sh
 		// PostgreSQL requires its directory to be writable by only itself.
 		// Pod "securityContext.fsGroup" sets g+w on directories for *some*
 		// storage providers. Ensure the current user owns the directory, and
-		// remove group permissions.
+		// remove group-write permission.
 		// - https://www.postgresql.org/docs/current/creating-cluster.html
 		// - https://git.postgresql.org/gitweb/?p=postgresql.git;f=src/backend/postmaster/postmaster.c;hb=REL_10_0#l1522
-		// - https://git.postgresql.org/gitweb/?p=postgresql.git;f=src/backend/utils/init/miscinit.c;hb=REL_14_0#l349
+		// - https://git.postgresql.org/gitweb/?p=postgresql.git;f=src/backend/utils/init/miscinit.c;hb=REL_11_0#l142
+		// - https://git.postgresql.org/gitweb/?p=postgresql.git;f=src/backend/utils/init/miscinit.c;hb=REL_17_0#l386
 		// - https://issue.k8s.io/93802#issuecomment-717646167
 		//
 		// When the directory does not exist, create it with the correct permissions.
 		// When the directory has the correct owner, set the correct permissions.
 		`if [[ ! -e "${postgres_data_directory}" || -O "${postgres_data_directory}" ]]; then`,
-		`install --directory --mode=0700 "${postgres_data_directory}"`,
+		`install --directory --mode=0750 "${postgres_data_directory}"`,
 		//
 		// The directory exists but its owner is wrong. When it is writable,
 		// the set-group-ID bit indicates that "fsGroup" probably ran on its
 		// contents making them safe to use. In this case, we can make a new
 		// directory (owned by this user) and refill it.
 		`elif [[ -w "${postgres_data_directory}" && -g "${postgres_data_directory}" ]]; then`,
-		`recreate "${postgres_data_directory}" '0700'`,
+		`recreate "${postgres_data_directory}" '0750'`,
 		//
 		// The directory exists, its owner is wrong, and it is not writable.
 		`else (halt Permissions!); fi ||`,

internal/postgres/reconcile_test.go

@@ -263,9 +263,9 @@ initContainers:
     [[ -d "${bootstrap_dir}" ]] && results 'bootstrap directory' "${bootstrap_dir}"
     [[ -d "${bootstrap_dir}" ]] && postgres_data_directory="${bootstrap_dir}"
     if [[ ! -e "${postgres_data_directory}" || -O "${postgres_data_directory}" ]]; then
-    install --directory --mode=0700 "${postgres_data_directory}"
+    install --directory --mode=0750 "${postgres_data_directory}"
     elif [[ -w "${postgres_data_directory}" && -g "${postgres_data_directory}" ]]; then
-    recreate "${postgres_data_directory}" '0700'
+    recreate "${postgres_data_directory}" '0750'
     else (halt Permissions!); fi ||
     halt "$(permissions "${postgres_data_directory}" ||:)"
     (mkdir -p '/pgdata/pgbackrest/log' && { chmod 0775 '/pgdata/pgbackrest/log' '/pgdata/pgbackrest' || :; }) ||

internal/registration/runner_test.go

@@ -20,9 +20,9 @@ import (
 	"gotest.tools/v3/assert"
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	"k8s.io/client-go/kubernetes/scheme"
 	"sigs.k8s.io/controller-runtime/pkg/manager"
 
+	"github.com/crunchydata/postgres-operator/internal/controller/runtime"
 	"github.com/crunchydata/postgres-operator/internal/testing/events"
 )
 
@@ -381,7 +381,7 @@ func TestRunnerRequiredEvents(t *testing.T) {
 
 					conditions := append([]metav1.Condition{}, tt.before...)
 					object := &corev1.ConfigMap{}
-					recorder := events.NewRecorder(t, scheme.Scheme)
+					recorder := events.NewRecorder(t, runtime.Scheme)
 
 					result := r.Required(recorder, object, &conditions)
 
@@ -413,7 +413,7 @@ func TestRunnerRequiredEvents(t *testing.T) {
 			} {
 				conditions := append([]metav1.Condition{}, tt.before...)
 				object := &corev1.ConfigMap{}
-				recorder := events.NewRecorder(t, scheme.Scheme)
+				recorder := events.NewRecorder(t, runtime.Scheme)
 
 				result := r.Required(recorder, object, &conditions)
 
@@ -441,7 +441,7 @@ func TestRunnerRequiredEvents(t *testing.T) {
 			} {
 				conditions := append([]metav1.Condition{}, tt.before...)
 				object := &corev1.ConfigMap{}
-				recorder := events.NewRecorder(t, scheme.Scheme)
+				recorder := events.NewRecorder(t, runtime.Scheme)
 
 				result := r.Required(recorder, object, &conditions)
 
@@ -475,7 +475,7 @@ func TestRunnerRequiredEvents(t *testing.T) {
 
 					conditions := append([]metav1.Condition{}, tt.before...)
 					object := &corev1.ConfigMap{}
-					recorder := events.NewRecorder(t, scheme.Scheme)
+					recorder := events.NewRecorder(t, runtime.Scheme)
 
 					result := r.Required(recorder, object, &conditions)
 
@@ -508,7 +508,7 @@ func TestRunnerRequiredEvents(t *testing.T) {
 			} {
 				conditions := append([]metav1.Condition{}, tt.before...)
 				object := &corev1.ConfigMap{}
-				recorder := events.NewRecorder(t, scheme.Scheme)
+				recorder := events.NewRecorder(t, runtime.Scheme)
 
 				result := r.Required(recorder, object, &conditions)
 

internal/testing/require/kubernetes.go

@@ -139,14 +139,14 @@ func kubernetes3(t TestingT) (*envtest.Environment, client.Client) {
 		// Calculate the project directory as reported by [goruntime.CallersFrames].
 		frame, ok := frames.Next()
 		self := frame.File
-		root := strings.TrimSuffix(self,
-			filepath.Join("internal", "testing", "require", "kubernetes.go"))
+		root := Value(filepath.EvalSymlinks(strings.TrimSuffix(self,
+			filepath.Join("internal", "testing", "require", "kubernetes.go"))))
 
 		// Find the first caller that is not in this file.
 		for ok && frame.File == self {
 			frame, ok = frames.Next()
 		}
-		caller := frame.File
+		caller := Value(filepath.EvalSymlinks(frame.File))
 
 		// Calculate the project directory path relative to the caller.
 		base := Value(filepath.Rel(filepath.Dir(caller), root))
@@ -159,8 +159,7 @@ func kubernetes3(t TestingT) (*envtest.Environment, client.Client) {
 		); assert.Check(t,
 			err == nil && len(pkgs) > 0 && pkgs[0].Module != nil, "got %v\n%#v", err, pkgs,
 		) {
-			snapshotter, err = filepath.Rel(root, pkgs[0].Module.Dir)
-			assert.NilError(t, err)
+			snapshotter = Value(filepath.Rel(root, pkgs[0].Module.Dir))
 		}
 
 		env := EnvTest(t, envtest.CRDInstallOptions{