Migration assistance

.gitignore

@@ -1,5 +1,4 @@
 .DS_Store
 /vendor/
 tools
-/testing/kuttl/e2e-generated/
-/testing/kuttl/e2e-generated-other/
+/testing/kuttl/e2e-generated*/

Makefile

@@ -226,7 +226,14 @@ generate-kuttl:
 	[ ! -d testing/kuttl/e2e-generated ] || rm -r testing/kuttl/e2e-generated
 	[ ! -d testing/kuttl/e2e-generated-other ] || rm -r testing/kuttl/e2e-generated-other
 	bash -ceu ' \
-	render() { envsubst '"'"'$$KUTTL_PG_VERSION $$KUTTL_POSTGIS_VERSION $$KUTTL_PSQL_IMAGE'"'"'; }; \
+	case $(KUTTL_PG_VERSION) in \
+	15 ) export KUTTL_BITNAMI_IMAGE_TAG=15.0.0-debian-11-r4 ;; \
+	14 ) export KUTTL_BITNAMI_IMAGE_TAG=14.5.0-debian-11-r37 ;; \
+	13 ) export KUTTL_BITNAMI_IMAGE_TAG=13.8.0-debian-11-r39 ;; \
+	12 ) export KUTTL_BITNAMI_IMAGE_TAG=12.12.0-debian-11-r40 ;; \
+	11 ) export KUTTL_BITNAMI_IMAGE_TAG=11.17.0-debian-11-r39 ;; \
+	esac; \
+	render() { envsubst '"'"'$$KUTTL_PG_VERSION $$KUTTL_POSTGIS_VERSION $$KUTTL_PSQL_IMAGE $$KUTTL_BITNAMI_IMAGE_TAG'"'"'; }; \
 	while [ $$# -gt 0 ]; do \
 		source="$${1}" target="$${1/e2e/e2e-generated}"; \
 		mkdir -p "$${target%/*}"; render < "$${source}" > "$${target}"; \

internal/postgres/config.go

@@ -26,6 +26,27 @@ import (
 )
 
 const (
+	// bashHalt is a Bash function that prints its arguments to stderr then
+	// exits with a non-zero status. It uses the exit status of the prior
+	// command if that was not zero.
+	bashHalt = `halt() { local rc=$?; >&2 echo "$@"; exit "${rc/#0/1}"; }`
+
+	// bashPermissions is a Bash function that prints the permissions of a file
+	// or directory and all its parent directories, except the root directory.
+	bashPermissions = `permissions() {` +
+		` while [[ -n "$1" ]]; do set "${1%/*}" "$@"; done; shift;` +
+		` stat -Lc '%A %4u %4g %n' "$@";` +
+		` }`
+
+	// bashRecreateDirectory is a Bash function that moves the contents of an
+	// existing directory into a newly created directory of the same name.
+	bashRecreateDirectory = `
+recreate() (
+  local tmp; tmp=$(mktemp -d -p "${1%/*}"); GLOBIGNORE='.:..'; set -x
+  chmod "$2" "${tmp}"; mv "$1"/* "${tmp}"; rmdir "$1"; mv "${tmp}" "$1"
+)
+`
+
 	// bashSafeLink is a Bash function that moves an existing file or directory
 	// and replaces it with a symbolic link.
 	bashSafeLink = `
@@ -184,10 +205,19 @@ func startupCommand(
 	script := strings.Join([]string{
 		`declare -r expected_major_version="$1" pgwal_directory="$2" pgbrLog_directory="$3"`,
 
+		// Function to print the permissions of a file or directory and its parents.
+		bashPermissions,
+
+		// Function to print a message to stderr then exit non-zero.
+		bashHalt,
+
 		// Function to log values in a basic structured format.
 		`results() { printf '::postgres-operator: %s::%s\n' "$@"; }`,
 
-		// Function to change a directory symlink while keeping the directory content.
+		// Function to change the owner of an existing directory.
+		strings.TrimSpace(bashRecreateDirectory),
+
+		// Function to change a directory symlink while keeping the directory contents.
 		strings.TrimSpace(bashSafeLink),
 
 		// Log the effective user ID and all the group IDs.
@@ -198,14 +228,16 @@ func startupCommand(
 		// match the cluster spec.
 		`results 'postgres path' "$(command -v postgres)"`,
 		`results 'postgres version' "${postgres_version:=$(postgres --version)}"`,
-		`[[ "${postgres_version}" == *") ${expected_major_version}."* ]]`,
+		`[[ "${postgres_version}" =~ ") ${expected_major_version}"($|[^0-9]) ]] ||`,
+		`halt Expected PostgreSQL version "${expected_major_version}"`,
 
 		// Abort when the configured data directory is not $PGDATA.
 		// - https://www.postgresql.org/docs/current/runtime-config-file-locations.html
 		`results 'config directory' "${PGDATA:?}"`,
 		`postgres_data_directory=$([ -d "${PGDATA}" ] && postgres -C data_directory || echo "${PGDATA}")`,
 		`results 'data directory' "${postgres_data_directory}"`,
-		`[ "${postgres_data_directory}" = "${PGDATA}" ]`,
+		`[[ "${postgres_data_directory}" == "${PGDATA}" ]] ||`,
+		`halt Expected matching config and data directories`,
 
 		// Determine if the data directory has been prepared for bootstrapping the cluster
 		`bootstrap_dir="${postgres_data_directory}_bootstrap"`,
@@ -214,15 +246,33 @@ func startupCommand(
 
 		// PostgreSQL requires its directory to be writable by only itself.
 		// Pod "securityContext.fsGroup" sets g+w on directories for *some*
-		// storage providers.
+		// storage providers. Ensure the current user owns the directory, and
+		// remove group permissions.
 		// - https://www.postgresql.org/docs/current/creating-cluster.html
-		// - https://git.postgresql.org/gitweb/?p=postgresql.git;f=src/backend/utils/init/miscinit.c;hb=REL_13_0#l319
+		// - 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://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}"`,
+		//
+		// 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'`,
+		//
+		// The directory exists, its owner is wrong, and it is not writable.
+		`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}"`,
+		`install --directory --mode=0775 "${pgbrLog_directory}" ||`,
+		`halt "$(permissions "${pgbrLog_directory}" ||:)"`,
 
 		// Copy replication client certificate files
 		// from the /pgconf/tls/replication directory to the /tmp/replication directory in order
@@ -243,7 +293,15 @@ func startupCommand(
 		// Abort when the data directory is not empty and its version does not
 		// match the cluster spec.
 		`results 'data version' "${postgres_data_version:=$(< "${postgres_data_directory}/PG_VERSION")}"`,
-		`[ "${postgres_data_version}" = "${expected_major_version}" ]`,
+		`[[ "${postgres_data_version}" == "${expected_major_version}" ]] ||`,
+		`halt Expected PostgreSQL data version "${expected_major_version}"`,
+
+		// For a restore from datasource:
+		// Patroni will complain if there's no `postgresql.conf` file
+		// and PGDATA may be missing that file if this is a restored database
+		// where the conf file was kept elsewhere.
+		`[[ ! -f "${postgres_data_directory}/postgresql.conf" ]] &&`,
+		`touch "${postgres_data_directory}/postgresql.conf"`,
 
 		// Safely move the WAL directory onto the intended volume. PostgreSQL
 		// always writes WAL files in the "pg_wal" directory inside the data

internal/postgres/config_test.go

@@ -16,6 +16,9 @@
 package postgres
 
 import (
+	"bytes"
+	"errors"
+	"fmt"
 	"os"
 	"os/exec"
 	"path/filepath"
@@ -26,6 +29,7 @@ import (
 	corev1 "k8s.io/api/core/v1"
 	"sigs.k8s.io/yaml"
 
+	"github.com/crunchydata/postgres-operator/internal/testing/cmp"
 	"github.com/crunchydata/postgres-operator/internal/testing/require"
 	"github.com/crunchydata/postgres-operator/pkg/apis/postgres-operator.crunchydata.com/v1beta1"
 )
@@ -57,6 +61,151 @@ func TestWALDirectory(t *testing.T) {
 	assert.Equal(t, WALDirectory(cluster, instance), "/pgwal/pg13_wal")
 }
 
+func TestBashHalt(t *testing.T) {
+	t.Run("NoPipeline", func(t *testing.T) {
+		cmd := exec.Command("bash")
+		cmd.Args = append(cmd.Args, "-c", "--", bashHalt+`; halt ab cd e`)
+
+		var exit *exec.ExitError
+		stdout, err := cmd.Output()
+		assert.Assert(t, errors.As(err, &exit))
+		assert.Equal(t, string(stdout), "", "expected no stdout")
+		assert.Equal(t, string(exit.Stderr), "ab cd e\n")
+		assert.Equal(t, exit.ExitCode(), 1)
+	})
+
+	t.Run("PipelineZeroStatus", func(t *testing.T) {
+		cmd := exec.Command("bash")
+		cmd.Args = append(cmd.Args, "-c", "--", bashHalt+`; true && halt message`)
+
+		var exit *exec.ExitError
+		stdout, err := cmd.Output()
+		assert.Assert(t, errors.As(err, &exit))
+		assert.Equal(t, string(stdout), "", "expected no stdout")
+		assert.Equal(t, string(exit.Stderr), "message\n")
+		assert.Equal(t, exit.ExitCode(), 1)
+	})
+
+	t.Run("PipelineNonZeroStatus", func(t *testing.T) {
+		cmd := exec.Command("bash")
+		cmd.Args = append(cmd.Args, "-c", "--", bashHalt+`; (exit 99) || halt $'multi\nline'`)
+
+		var exit *exec.ExitError
+		stdout, err := cmd.Output()
+		assert.Assert(t, errors.As(err, &exit))
+		assert.Equal(t, string(stdout), "", "expected no stdout")
+		assert.Equal(t, string(exit.Stderr), "multi\nline\n")
+		assert.Equal(t, exit.ExitCode(), 99)
+	})
+
+	t.Run("Subshell", func(t *testing.T) {
+		cmd := exec.Command("bash")
+		cmd.Args = append(cmd.Args, "-c", "--", bashHalt+`; (halt 'err') || echo 'after'`)
+
+		stderr := new(bytes.Buffer)
+		cmd.Stderr = stderr
+
+		stdout, err := cmd.Output()
+		assert.NilError(t, err)
+		assert.Equal(t, string(stdout), "after\n")
+		assert.Equal(t, stderr.String(), "err\n")
+		assert.Equal(t, cmd.ProcessState.ExitCode(), 0)
+	})
+}
+
+func TestBashPermissions(t *testing.T) {
+	// macOS `stat` takes different arguments than BusyBox and GNU coreutils.
+	if output, err := exec.Command("stat", "--help").CombinedOutput(); err != nil {
+		t.Skip(`requires "stat" executable`)
+	} else if !strings.Contains(string(output), "%A") {
+		t.Skip(`requires "stat" with access format sequence`)
+	}
+
+	dir := t.TempDir()
+	assert.NilError(t, os.Mkdir(filepath.Join(dir, "sub"), 0o751))
+	assert.NilError(t, os.Chmod(filepath.Join(dir, "sub"), 0o751))
+	assert.NilError(t, os.WriteFile(filepath.Join(dir, "sub", "fn"), nil, 0o624)) // #nosec G306 OK permissions for a temp dir in a test
+	assert.NilError(t, os.Chmod(filepath.Join(dir, "sub", "fn"), 0o624))
+
+	cmd := exec.Command("bash")
+	cmd.Args = append(cmd.Args, "-c", "--",
+		bashPermissions+`; permissions "$@"`, "-",
+		filepath.Join(dir, "sub", "fn"))
+
+	stdout, err := cmd.Output()
+	assert.NilError(t, err)
+	assert.Assert(t, cmp.Regexp(``+
+		`drwxr-x--x\s+\d+\s+\d+\s+[^ ]+/sub\n`+
+		`-rw--w-r--\s+\d+\s+\d+\s+[^ ]+/sub/fn\n`+
+		`$`, string(stdout)))
+}
+
+func TestBashRecreateDirectory(t *testing.T) {
+	// macOS `stat` takes different arguments than BusyBox and GNU coreutils.
+	if output, err := exec.Command("stat", "--help").CombinedOutput(); err != nil {
+		t.Skip(`requires "stat" executable`)
+	} else if !strings.Contains(string(output), "%a") {
+		t.Skip(`requires "stat" with access format sequence`)
+	}
+
+	dir := t.TempDir()
+	assert.NilError(t, os.Mkdir(filepath.Join(dir, "d"), 0o755))
+	assert.NilError(t, os.WriteFile(filepath.Join(dir, "d", ".hidden"), nil, 0o644)) // #nosec G306 OK permissions for a temp dir in a test
+	assert.NilError(t, os.WriteFile(filepath.Join(dir, "d", "file"), nil, 0o644))    // #nosec G306 OK permissions for a temp dir in a test
+
+	stat := func(args ...string) string {
+		cmd := exec.Command("stat", "-c", "%i %#a %N")
+		cmd.Args = append(cmd.Args, args...)
+		out, err := cmd.CombinedOutput()
+
+		t.Helper()
+		assert.NilError(t, err, string(out))
+		return string(out)
+	}
+
+	var before, after struct{ d, f, dInode, dPerms string }
+
+	before.d = stat(filepath.Join(dir, "d"))
+	before.f = stat(
+		filepath.Join(dir, "d", ".hidden"),
+		filepath.Join(dir, "d", "file"),
+	)
+
+	cmd := exec.Command("bash")
+	cmd.Args = append(cmd.Args, "-ceu", "--",
+		bashRecreateDirectory+` recreate "$@"`, "-",
+		filepath.Join(dir, "d"), "0740")
+
+	output, err := cmd.CombinedOutput()
+	assert.NilError(t, err, string(output))
+	assert.Assert(t, cmp.Regexp(`^`+
+		`[+] chmod 0740 [^ ]+/tmp.[^ /]+\n`+
+		`[+] mv [^ ]+/d/.hidden [^ ]+/d/file [^ ]+/tmp.[^ /]+\n`+
+		`[+] rmdir [^ ]+/d\n`+
+		`[+] mv [^ ]+/tmp.[^ /]+ [^ ]+/d\n`+
+		`$`, string(output)))
+
+	after.d = stat(filepath.Join(dir, "d"))
+	after.f = stat(
+		filepath.Join(dir, "d", ".hidden"),
+		filepath.Join(dir, "d", "file"),
+	)
+
+	_, err = fmt.Sscan(before.d, &before.dInode, &before.dPerms)
+	assert.NilError(t, err)
+	_, err = fmt.Sscan(after.d, &after.dInode, &after.dPerms)
+	assert.NilError(t, err)
+
+	// New directory is new.
+	assert.Assert(t, after.dInode != before.dInode)
+
+	// New directory has the requested permissions.
+	assert.Equal(t, after.dPerms, "0740")
+
+	// Files are in the new directory and unchanged.
+	assert.DeepEqual(t, after.f, before.f)
+}
+
 func TestBashSafeLink(t *testing.T) {
 	// macOS lacks `realpath` which is part of GNU coreutils.
 	if _, err := exec.LookPath("realpath"); err != nil {
@@ -310,13 +459,6 @@ func TestBashSafeLink(t *testing.T) {
 	})
 }
 
-func TestBashSafeLinkPrettyYAML(t *testing.T) {
-	b, err := yaml.Marshal(bashSafeLink)
-	assert.NilError(t, err)
-	assert.Assert(t, strings.HasPrefix(string(b), `|`),
-		"expected literal block scalar, got:\n%s", b)
-}
-
 func TestStartupCommand(t *testing.T) {
 	shellcheck := require.ShellCheck(t)
 
@@ -329,14 +471,22 @@ func TestStartupCommand(t *testing.T) {
 	// 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]
 
 	// Write out that inline script.
 	dir := t.TempDir()
 	file := filepath.Join(dir, "script.bash")
-	assert.NilError(t, os.WriteFile(file, []byte(command[3]), 0o600))
+	assert.NilError(t, os.WriteFile(file, []byte(script), 0o600))
 
 	// Expect shellcheck to be happy.
 	cmd := exec.Command(shellcheck, "--enable=all", file)
 	output, err := cmd.CombinedOutput()
 	assert.NilError(t, err, "%q\n%s", cmd.Args, output)
+
+	t.Run("PrettyYAML", func(t *testing.T) {
+		b, err := yaml.Marshal(script)
+		assert.NilError(t, err)
+		assert.Assert(t, strings.HasPrefix(string(b), `|`),
+			"expected literal block scalar, got:\n%s", b)
+	})
 }