Skip to content

Commit 80dfa39

Browse files
authored
Migration assistance (#3445)
* Log errors when the PostgreSQL data directory is wrong The postgres-startup container now reports when it finds the installed PostgreSQL binaries do not match the specified PostgreSQL version. Some storage providers do not mount the PostgreSQL data volume with correct ownership or permissions. The postgres-startup container now prints those attributes of parent directories when it cannot create or modify a needed file or directory. Issue: [sc-11804] Issue: #2870 Co-authored-by: @cbandy * Change owner of the PostgreSQL directory at startup PostgreSQL won't to start unless it owns the data directory. Kubernetes sets the group according to fsGroup but not the owner. The postgres-startup container now recreates the data directory to give it a new owner when permissions are sufficient to do so. It now raises an error when the owner is incorrect and cannot be changed. Issue: [sc-15909] See: https://docs.k8s.io/tasks/configure-pod-container/security-context/ Co-authored-by: @cbandy * Add KUTTL test for migration from third-party PGSQL Issue: [sc-15909]
1 parent 0620cfc commit 80dfa39

18 files changed

+691
-22
lines changed

.gitignore

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,4 @@
11
.DS_Store
22
/vendor/
33
tools
4-
/testing/kuttl/e2e-generated/
5-
/testing/kuttl/e2e-generated-other/
4+
/testing/kuttl/e2e-generated*/

Makefile

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -227,7 +227,14 @@ generate-kuttl:
227227
[ ! -d testing/kuttl/e2e-generated ] || rm -r testing/kuttl/e2e-generated
228228
[ ! -d testing/kuttl/e2e-generated-other ] || rm -r testing/kuttl/e2e-generated-other
229229
bash -ceu ' \
230-
render() { envsubst '"'"'$$KUTTL_PG_VERSION $$KUTTL_POSTGIS_VERSION $$KUTTL_PSQL_IMAGE'"'"'; }; \
230+
case $(KUTTL_PG_VERSION) in \
231+
15 ) export KUTTL_BITNAMI_IMAGE_TAG=15.0.0-debian-11-r4 ;; \
232+
14 ) export KUTTL_BITNAMI_IMAGE_TAG=14.5.0-debian-11-r37 ;; \
233+
13 ) export KUTTL_BITNAMI_IMAGE_TAG=13.8.0-debian-11-r39 ;; \
234+
12 ) export KUTTL_BITNAMI_IMAGE_TAG=12.12.0-debian-11-r40 ;; \
235+
11 ) export KUTTL_BITNAMI_IMAGE_TAG=11.17.0-debian-11-r39 ;; \
236+
esac; \
237+
render() { envsubst '"'"'$$KUTTL_PG_VERSION $$KUTTL_POSTGIS_VERSION $$KUTTL_PSQL_IMAGE $$KUTTL_BITNAMI_IMAGE_TAG'"'"'; }; \
231238
while [ $$# -gt 0 ]; do \
232239
source="$${1}" target="$${1/e2e/e2e-generated}"; \
233240
mkdir -p "$${target%/*}"; render < "$${source}" > "$${target}"; \

internal/postgres/config.go

Lines changed: 65 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,27 @@ import (
2626
)
2727

2828
const (
29+
// bashHalt is a Bash function that prints its arguments to stderr then
30+
// exits with a non-zero status. It uses the exit status of the prior
31+
// command if that was not zero.
32+
bashHalt = `halt() { local rc=$?; >&2 echo "$@"; exit "${rc/#0/1}"; }`
33+
34+
// bashPermissions is a Bash function that prints the permissions of a file
35+
// or directory and all its parent directories, except the root directory.
36+
bashPermissions = `permissions() {` +
37+
` while [[ -n "$1" ]]; do set "${1%/*}" "$@"; done; shift;` +
38+
` stat -Lc '%A %4u %4g %n' "$@";` +
39+
` }`
40+
41+
// bashRecreateDirectory is a Bash function that moves the contents of an
42+
// existing directory into a newly created directory of the same name.
43+
bashRecreateDirectory = `
44+
recreate() (
45+
local tmp; tmp=$(mktemp -d -p "${1%/*}"); GLOBIGNORE='.:..'; set -x
46+
chmod "$2" "${tmp}"; mv "$1"/* "${tmp}"; rmdir "$1"; mv "${tmp}" "$1"
47+
)
48+
`
49+
2950
// bashSafeLink is a Bash function that moves an existing file or directory
3051
// and replaces it with a symbolic link.
3152
bashSafeLink = `
@@ -184,10 +205,19 @@ func startupCommand(
184205
script := strings.Join([]string{
185206
`declare -r expected_major_version="$1" pgwal_directory="$2" pgbrLog_directory="$3"`,
186207

208+
// Function to print the permissions of a file or directory and its parents.
209+
bashPermissions,
210+
211+
// Function to print a message to stderr then exit non-zero.
212+
bashHalt,
213+
187214
// Function to log values in a basic structured format.
188215
`results() { printf '::postgres-operator: %s::%s\n' "$@"; }`,
189216

190-
// Function to change a directory symlink while keeping the directory content.
217+
// Function to change the owner of an existing directory.
218+
strings.TrimSpace(bashRecreateDirectory),
219+
220+
// Function to change a directory symlink while keeping the directory contents.
191221
strings.TrimSpace(bashSafeLink),
192222

193223
// Log the effective user ID and all the group IDs.
@@ -198,14 +228,16 @@ func startupCommand(
198228
// match the cluster spec.
199229
`results 'postgres path' "$(command -v postgres)"`,
200230
`results 'postgres version' "${postgres_version:=$(postgres --version)}"`,
201-
`[[ "${postgres_version}" == *") ${expected_major_version}."* ]]`,
231+
`[[ "${postgres_version}" =~ ") ${expected_major_version}"($|[^0-9]) ]] ||`,
232+
`halt Expected PostgreSQL version "${expected_major_version}"`,
202233

203234
// Abort when the configured data directory is not $PGDATA.
204235
// - https://www.postgresql.org/docs/current/runtime-config-file-locations.html
205236
`results 'config directory' "${PGDATA:?}"`,
206237
`postgres_data_directory=$([ -d "${PGDATA}" ] && postgres -C data_directory || echo "${PGDATA}")`,
207238
`results 'data directory' "${postgres_data_directory}"`,
208-
`[ "${postgres_data_directory}" = "${PGDATA}" ]`,
239+
`[[ "${postgres_data_directory}" == "${PGDATA}" ]] ||`,
240+
`halt Expected matching config and data directories`,
209241

210242
// Determine if the data directory has been prepared for bootstrapping the cluster
211243
`bootstrap_dir="${postgres_data_directory}_bootstrap"`,
@@ -214,15 +246,33 @@ func startupCommand(
214246

215247
// PostgreSQL requires its directory to be writable by only itself.
216248
// Pod "securityContext.fsGroup" sets g+w on directories for *some*
217-
// storage providers.
249+
// storage providers. Ensure the current user owns the directory, and
250+
// remove group permissions.
218251
// - https://www.postgresql.org/docs/current/creating-cluster.html
219-
// - https://git.postgresql.org/gitweb/?p=postgresql.git;f=src/backend/utils/init/miscinit.c;hb=REL_13_0#l319
252+
// - https://git.postgresql.org/gitweb/?p=postgresql.git;f=src/backend/postmaster/postmaster.c;hb=REL_10_0#l1522
253+
// - https://git.postgresql.org/gitweb/?p=postgresql.git;f=src/backend/utils/init/miscinit.c;hb=REL_14_0#l349
220254
// - https://issue.k8s.io/93802#issuecomment-717646167
255+
//
256+
// When the directory does not exist, create it with the correct permissions.
257+
// When the directory has the correct owner, set the correct permissions.
258+
`if [[ ! -e "${postgres_data_directory}" || -O "${postgres_data_directory}" ]]; then`,
221259
`install --directory --mode=0700 "${postgres_data_directory}"`,
260+
//
261+
// The directory exists but its owner is wrong. When it is writable,
262+
// the set-group-ID bit indicates that "fsGroup" probably ran on its
263+
// contents making them safe to use. In this case, we can make a new
264+
// directory (owned by this user) and refill it.
265+
`elif [[ -w "${postgres_data_directory}" && -g "${postgres_data_directory}" ]]; then`,
266+
`recreate "${postgres_data_directory}" '0700'`,
267+
//
268+
// The directory exists, its owner is wrong, and it is not writable.
269+
`else (halt Permissions!); fi ||`,
270+
`halt "$(permissions "${postgres_data_directory}" ||:)"`,
222271

223272
// Create the pgBackRest log directory.
224273
`results 'pgBackRest log directory' "${pgbrLog_directory}"`,
225-
`install --directory --mode=0775 "${pgbrLog_directory}"`,
274+
`install --directory --mode=0775 "${pgbrLog_directory}" ||`,
275+
`halt "$(permissions "${pgbrLog_directory}" ||:)"`,
226276

227277
// Copy replication client certificate files
228278
// from the /pgconf/tls/replication directory to the /tmp/replication directory in order
@@ -243,7 +293,15 @@ func startupCommand(
243293
// Abort when the data directory is not empty and its version does not
244294
// match the cluster spec.
245295
`results 'data version' "${postgres_data_version:=$(< "${postgres_data_directory}/PG_VERSION")}"`,
246-
`[ "${postgres_data_version}" = "${expected_major_version}" ]`,
296+
`[[ "${postgres_data_version}" == "${expected_major_version}" ]] ||`,
297+
`halt Expected PostgreSQL data version "${expected_major_version}"`,
298+
299+
// For a restore from datasource:
300+
// Patroni will complain if there's no `postgresql.conf` file
301+
// and PGDATA may be missing that file if this is a restored database
302+
// where the conf file was kept elsewhere.
303+
`[[ ! -f "${postgres_data_directory}/postgresql.conf" ]] &&`,
304+
`touch "${postgres_data_directory}/postgresql.conf"`,
247305

248306
// Safely move the WAL directory onto the intended volume. PostgreSQL
249307
// always writes WAL files in the "pg_wal" directory inside the data

internal/postgres/config_test.go

Lines changed: 158 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,9 @@
1616
package postgres
1717

1818
import (
19+
"bytes"
20+
"errors"
21+
"fmt"
1922
"os"
2023
"os/exec"
2124
"path/filepath"
@@ -26,6 +29,7 @@ import (
2629
corev1 "k8s.io/api/core/v1"
2730
"sigs.k8s.io/yaml"
2831

32+
"github.com/crunchydata/postgres-operator/internal/testing/cmp"
2933
"github.com/crunchydata/postgres-operator/internal/testing/require"
3034
"github.com/crunchydata/postgres-operator/pkg/apis/postgres-operator.crunchydata.com/v1beta1"
3135
)
@@ -57,6 +61,151 @@ func TestWALDirectory(t *testing.T) {
5761
assert.Equal(t, WALDirectory(cluster, instance), "/pgwal/pg13_wal")
5862
}
5963

64+
func TestBashHalt(t *testing.T) {
65+
t.Run("NoPipeline", func(t *testing.T) {
66+
cmd := exec.Command("bash")
67+
cmd.Args = append(cmd.Args, "-c", "--", bashHalt+`; halt ab cd e`)
68+
69+
var exit *exec.ExitError
70+
stdout, err := cmd.Output()
71+
assert.Assert(t, errors.As(err, &exit))
72+
assert.Equal(t, string(stdout), "", "expected no stdout")
73+
assert.Equal(t, string(exit.Stderr), "ab cd e\n")
74+
assert.Equal(t, exit.ExitCode(), 1)
75+
})
76+
77+
t.Run("PipelineZeroStatus", func(t *testing.T) {
78+
cmd := exec.Command("bash")
79+
cmd.Args = append(cmd.Args, "-c", "--", bashHalt+`; true && halt message`)
80+
81+
var exit *exec.ExitError
82+
stdout, err := cmd.Output()
83+
assert.Assert(t, errors.As(err, &exit))
84+
assert.Equal(t, string(stdout), "", "expected no stdout")
85+
assert.Equal(t, string(exit.Stderr), "message\n")
86+
assert.Equal(t, exit.ExitCode(), 1)
87+
})
88+
89+
t.Run("PipelineNonZeroStatus", func(t *testing.T) {
90+
cmd := exec.Command("bash")
91+
cmd.Args = append(cmd.Args, "-c", "--", bashHalt+`; (exit 99) || halt $'multi\nline'`)
92+
93+
var exit *exec.ExitError
94+
stdout, err := cmd.Output()
95+
assert.Assert(t, errors.As(err, &exit))
96+
assert.Equal(t, string(stdout), "", "expected no stdout")
97+
assert.Equal(t, string(exit.Stderr), "multi\nline\n")
98+
assert.Equal(t, exit.ExitCode(), 99)
99+
})
100+
101+
t.Run("Subshell", func(t *testing.T) {
102+
cmd := exec.Command("bash")
103+
cmd.Args = append(cmd.Args, "-c", "--", bashHalt+`; (halt 'err') || echo 'after'`)
104+
105+
stderr := new(bytes.Buffer)
106+
cmd.Stderr = stderr
107+
108+
stdout, err := cmd.Output()
109+
assert.NilError(t, err)
110+
assert.Equal(t, string(stdout), "after\n")
111+
assert.Equal(t, stderr.String(), "err\n")
112+
assert.Equal(t, cmd.ProcessState.ExitCode(), 0)
113+
})
114+
}
115+
116+
func TestBashPermissions(t *testing.T) {
117+
// macOS `stat` takes different arguments than BusyBox and GNU coreutils.
118+
if output, err := exec.Command("stat", "--help").CombinedOutput(); err != nil {
119+
t.Skip(`requires "stat" executable`)
120+
} else if !strings.Contains(string(output), "%A") {
121+
t.Skip(`requires "stat" with access format sequence`)
122+
}
123+
124+
dir := t.TempDir()
125+
assert.NilError(t, os.Mkdir(filepath.Join(dir, "sub"), 0o751))
126+
assert.NilError(t, os.Chmod(filepath.Join(dir, "sub"), 0o751))
127+
assert.NilError(t, os.WriteFile(filepath.Join(dir, "sub", "fn"), nil, 0o624)) // #nosec G306 OK permissions for a temp dir in a test
128+
assert.NilError(t, os.Chmod(filepath.Join(dir, "sub", "fn"), 0o624))
129+
130+
cmd := exec.Command("bash")
131+
cmd.Args = append(cmd.Args, "-c", "--",
132+
bashPermissions+`; permissions "$@"`, "-",
133+
filepath.Join(dir, "sub", "fn"))
134+
135+
stdout, err := cmd.Output()
136+
assert.NilError(t, err)
137+
assert.Assert(t, cmp.Regexp(``+
138+
`drwxr-x--x\s+\d+\s+\d+\s+[^ ]+/sub\n`+
139+
`-rw--w-r--\s+\d+\s+\d+\s+[^ ]+/sub/fn\n`+
140+
`$`, string(stdout)))
141+
}
142+
143+
func TestBashRecreateDirectory(t *testing.T) {
144+
// macOS `stat` takes different arguments than BusyBox and GNU coreutils.
145+
if output, err := exec.Command("stat", "--help").CombinedOutput(); err != nil {
146+
t.Skip(`requires "stat" executable`)
147+
} else if !strings.Contains(string(output), "%a") {
148+
t.Skip(`requires "stat" with access format sequence`)
149+
}
150+
151+
dir := t.TempDir()
152+
assert.NilError(t, os.Mkdir(filepath.Join(dir, "d"), 0o755))
153+
assert.NilError(t, os.WriteFile(filepath.Join(dir, "d", ".hidden"), nil, 0o644)) // #nosec G306 OK permissions for a temp dir in a test
154+
assert.NilError(t, os.WriteFile(filepath.Join(dir, "d", "file"), nil, 0o644)) // #nosec G306 OK permissions for a temp dir in a test
155+
156+
stat := func(args ...string) string {
157+
cmd := exec.Command("stat", "-c", "%i %#a %N")
158+
cmd.Args = append(cmd.Args, args...)
159+
out, err := cmd.CombinedOutput()
160+
161+
t.Helper()
162+
assert.NilError(t, err, string(out))
163+
return string(out)
164+
}
165+
166+
var before, after struct{ d, f, dInode, dPerms string }
167+
168+
before.d = stat(filepath.Join(dir, "d"))
169+
before.f = stat(
170+
filepath.Join(dir, "d", ".hidden"),
171+
filepath.Join(dir, "d", "file"),
172+
)
173+
174+
cmd := exec.Command("bash")
175+
cmd.Args = append(cmd.Args, "-ceu", "--",
176+
bashRecreateDirectory+` recreate "$@"`, "-",
177+
filepath.Join(dir, "d"), "0740")
178+
179+
output, err := cmd.CombinedOutput()
180+
assert.NilError(t, err, string(output))
181+
assert.Assert(t, cmp.Regexp(`^`+
182+
`[+] chmod 0740 [^ ]+/tmp.[^ /]+\n`+
183+
`[+] mv [^ ]+/d/.hidden [^ ]+/d/file [^ ]+/tmp.[^ /]+\n`+
184+
`[+] rmdir [^ ]+/d\n`+
185+
`[+] mv [^ ]+/tmp.[^ /]+ [^ ]+/d\n`+
186+
`$`, string(output)))
187+
188+
after.d = stat(filepath.Join(dir, "d"))
189+
after.f = stat(
190+
filepath.Join(dir, "d", ".hidden"),
191+
filepath.Join(dir, "d", "file"),
192+
)
193+
194+
_, err = fmt.Sscan(before.d, &before.dInode, &before.dPerms)
195+
assert.NilError(t, err)
196+
_, err = fmt.Sscan(after.d, &after.dInode, &after.dPerms)
197+
assert.NilError(t, err)
198+
199+
// New directory is new.
200+
assert.Assert(t, after.dInode != before.dInode)
201+
202+
// New directory has the requested permissions.
203+
assert.Equal(t, after.dPerms, "0740")
204+
205+
// Files are in the new directory and unchanged.
206+
assert.DeepEqual(t, after.f, before.f)
207+
}
208+
60209
func TestBashSafeLink(t *testing.T) {
61210
// macOS lacks `realpath` which is part of GNU coreutils.
62211
if _, err := exec.LookPath("realpath"); err != nil {
@@ -310,13 +459,6 @@ func TestBashSafeLink(t *testing.T) {
310459
})
311460
}
312461

313-
func TestBashSafeLinkPrettyYAML(t *testing.T) {
314-
b, err := yaml.Marshal(bashSafeLink)
315-
assert.NilError(t, err)
316-
assert.Assert(t, strings.HasPrefix(string(b), `|`),
317-
"expected literal block scalar, got:\n%s", b)
318-
}
319-
320462
func TestStartupCommand(t *testing.T) {
321463
shellcheck := require.ShellCheck(t)
322464

@@ -329,14 +471,22 @@ func TestStartupCommand(t *testing.T) {
329471
// Expect a bash command with an inline script.
330472
assert.DeepEqual(t, command[:3], []string{"bash", "-ceu", "--"})
331473
assert.Assert(t, len(command) > 3)
474+
script := command[3]
332475

333476
// Write out that inline script.
334477
dir := t.TempDir()
335478
file := filepath.Join(dir, "script.bash")
336-
assert.NilError(t, os.WriteFile(file, []byte(command[3]), 0o600))
479+
assert.NilError(t, os.WriteFile(file, []byte(script), 0o600))
337480

338481
// Expect shellcheck to be happy.
339482
cmd := exec.Command(shellcheck, "--enable=all", file)
340483
output, err := cmd.CombinedOutput()
341484
assert.NilError(t, err, "%q\n%s", cmd.Args, output)
485+
486+
t.Run("PrettyYAML", func(t *testing.T) {
487+
b, err := yaml.Marshal(script)
488+
assert.NilError(t, err)
489+
assert.Assert(t, strings.HasPrefix(string(b), `|`),
490+
"expected literal block scalar, got:\n%s", b)
491+
})
342492
}

0 commit comments

Comments
 (0)