diff --git a/changelog.d/gsta-318-ubi-ssh-proof.md b/changelog.d/gsta-318-ubi-ssh-proof.md new file mode 100644 index 000000000..9e52edc94 --- /dev/null +++ b/changelog.d/gsta-318-ubi-ssh-proof.md @@ -0,0 +1 @@ +- **The Ubicloud runner no longer destroys a VM another caller took under the same name.** With an explicit `-n` name, a second caller could take the name in the second between our name check and our create. If our create's 409 answer was lost, `up` found that caller's VM by name, adopted it, and its teardown destroyed it. Now a VM found after an unanswered or 5xx create is adopted with teardown off. Teardown turns on only once this call's own ssh key opens the VM; keys from the ssh agent or ssh config don't count. If the key never does (within `UBI_SSH_WAIT` seconds, default 300), `up` keeps the record and recovery bundle, exits `UBI_CREATE_UNKNOWN`, and leaves the VM to its TTL. `run` reports `left=unknown` for it instead of `left=0`. (GST-1463) diff --git a/docs/troubleshooting.md b/docs/troubleshooting.md index bdeb468be..21b94bbb8 100644 --- a/docs/troubleshooting.md +++ b/docs/troubleshooting.md @@ -7055,7 +7055,7 @@ These codes come from `scripts/ubicloud/ubi-runner.sh` (GST-1220). `RECOVERED`, ### `UBI_CREATE_UNKNOWN` -**Meaning.** `up` or `run` sent a create that Ubicloud answered with a 5xx or not at all, then looked the VM up by name for `UBI_CREATE_GRACE` seconds and could not settle whether it exists. The name answered 404 to the check before the create, so a VM that shows up is this call's. The VM's record and recovery bundle stay. **Fix.** Run `scripts/ubicloud/ubi-runner.sh list` or the printed `down ` command to find out, then rerun. This owner's reap destroys an unconfirmed VM once its `-t` TTL ends. +**Meaning.** `up` or `run` sent a create that Ubicloud answered with a 5xx or not at all, and could not settle whether the VM is this call's: either the lookup by name never answered within `UBI_CREATE_GRACE` seconds, or a VM answers to the name but this call's ssh key did not open it within `UBI_SSH_WAIT` seconds. Another caller can take an explicit `-n` name between the name check and the create, so a VM found after an unanswered create is not destroyed until this call's key proves it is ours. The VM's record and recovery bundle stay, nothing destroys it, and `run` prints `left=unknown`. **Fix.** Run `scripts/ubicloud/ubi-runner.sh list` to see the VM and its owner. Run `down ` only when you know the VM is yours, because `down` destroys whatever answers to the name. This owner's reap destroys an unconfirmed VM once its `-t` TTL ends. ### `UBI_ATTACH_UNKNOWN` diff --git a/scripts/ubicloud/ubi-runner.sh b/scripts/ubicloud/ubi-runner.sh index 369bfd727..5216c048b 100755 --- a/scripts/ubicloud/ubi-runner.sh +++ b/scripts/ubicloud/ubi-runner.sh @@ -42,7 +42,11 @@ # included), so it never destroys a VM this call did not create. A 5xx or no # answer is not a refusal: `up` looks the VM up by name for UBI_CREATE_GRACE # seconds, and when that cannot settle it either, it keeps the record and the recovery -# bundle and exits UBI_CREATE_UNKNOWN. `down` counts a VM as gone only +# bundle and exits UBI_CREATE_UNKNOWN. Another caller can take an explicit -n +# name between the check and the create, so a VM found that way is adopted +# with teardown off, and teardown is armed only once this call's ssh key opens +# it (within UBI_SSH_WAIT seconds, default 300); if the key never does, `up` +# exits UBI_CREATE_UNKNOWN and leaves the VM to its TTL. `down` counts a VM as gone only # on a readable answer: a failed or unreadable VM list or show is unknown, so # down exits non-zero and keeps the recovery bundle. The API key reaches curl # as a header file on a pipe, never in its argv. A create refused @@ -150,6 +154,7 @@ DEFAULT_LOCATION="${UBI_LOCATION:-eu-central-h1}" GC_HOURS="${UBI_GC_HOURS:-0}" API_TIMEOUT="${UBI_API_TIMEOUT:-120}" CREATE_GRACE="${UBI_CREATE_GRACE:-180}" +SSH_WAIT="${UBI_SSH_WAIT:-300}" DOWN_TIMEOUT="${UBI_DOWN_TIMEOUT:-900}" POLL="${UBI_POLL_SECONDS:-5}" ALLOWED_STORAGE="${UBI_ALLOWED_STORAGE:-320 640}" @@ -270,7 +275,7 @@ load() { } ssh_opts() { - echo -i "$KEY" -o StrictHostKeyChecking=no -o UserKnownHostsFile=/dev/null \ + echo -i "$KEY" -o IdentitiesOnly=yes -o StrictHostKeyChecking=no -o UserKnownHostsFile=/dev/null \ -o LogLevel=ERROR -o ServerAliveInterval=30 -o ServerAliveCountMax=6 -o ConnectTimeout=10 \ -o ControlMaster=auto -o "ControlPath=$(dirname "$KEY")/ctl" -o ControlPersist=300 } @@ -1341,7 +1346,7 @@ cmd_up() { case $code in 200) created=yes ;; 4[0-9][0-9]) created=no ;; - *) created=unknown ;; + *) created=unknown UP_NAME="" ;; esac if [ "$created" = no ]; then UP_NAME="" @@ -1357,7 +1362,9 @@ cmd_up() { [ -z "$interrupted" ] || exit 130 if [ "$created" = unknown ]; then # A 5xx or no answer (000) is not a refusal: the VM may exist. Look it up - # by name until it shows or UBI_CREATE_GRACE has passed. + # by name until it shows or UBI_CREATE_GRACE has passed. Another caller can + # take the name between the check and the create (a lost 409), so teardown + # stays off until this call's key opens the VM. log "create of $location/$name answered HTTP $code; looking it up by name before deciding" local seen_code while :; do @@ -1373,9 +1380,9 @@ cmd_up() { esac write_state "$name" "$location" "$size" "" "$created" "$create_at" case $created in - yes) bundle_save "$name"; log "create of $location/$name went through; continuing" ;; - no) cat "$dir/create.err" >&2; die "create failed (HTTP $code): $location/$name was still absent ${CREATE_GRACE}s after the request" ;; - unknown) die "UBI_CREATE_UNKNOWN: could not tell whether $location/$name exists (create answered HTTP $code, its lookup HTTP $seen_code); its record and recovery bundle stay, teardown destroys it if it shows up, and this owner's reap destroys it once its -t TTL ends; check with: $0 down $name" ;; + yes) bundle_save "$name"; log "create of $location/$name went through; teardown waits until this call's key opens it" ;; + no) cat "$dir/create.err" >&2; rm -rf "$dir"; forget_vm "$name"; [ -z "$RECOVERY_DIR" ] || rm -f "$RECOVERY_DIR/$OWNER/$name.bundle"; die "create failed (HTTP $code): $location/$name was still absent ${CREATE_GRACE}s after the request" ;; + unknown) die "UBI_CREATE_UNKNOWN: could not tell whether $location/$name exists (create answered HTTP $code, its lookup HTTP $seen_code); its record and recovery bundle stay, nothing destroys it, and this owner's reap destroys it once its -t TTL ends; check with: $0 list" ;; esac fi if [ "$created" = no ]; then @@ -1405,12 +1412,18 @@ cmd_up() { bundle_save "$name" load "$name" - deadline=$(( $(date +%s) + 300 )) + deadline=$(( $(date +%s) + SSH_WAIT )) + local proof=() + [ -n "$UP_NAME" ] || proof=(-F /dev/null) # shellcheck disable=SC2046 - until ssh $(ssh_opts) "ubi@$IP" true 2>/dev/null; do - [ "$(date +%s)" -lt "$deadline" ] || die "SSH did not come up on $IP" + until ssh ${proof[@]+"${proof[@]}"} $(ssh_opts) "ubi@$IP" true 2>/dev/null; do + if [ "$(date +%s)" -ge "$deadline" ]; then + [ -z "$UP_NAME" ] || die "SSH did not come up on $IP" + die "UBI_CREATE_UNKNOWN: $location/$name answers its name after a create that answered HTTP $code, but this call's key did not open it in ${SSH_WAIT}s, so it may be another caller's; nothing destroys it, its record and recovery bundle stay, and this owner's reap destroys it once its -t TTL ends; check with: $0 list" + fi sleep "$POLL" done + [ -n "$UP_NAME" ] || { UP_NAME=$name; log "this call's key opened $name; teardown armed"; } remote "$name" 'cloud-init status --wait >/dev/null 2>&1 || true' if [ -n "$sink" ]; then sink_start "$name" "$sink" ${sink_paths[@]+"${sink_paths[@]}"} @@ -1685,13 +1698,16 @@ run_teardown() { else left=unknown others=unknown fi - [ -n "$UP_NAME" ] || left=0 + if [ -z "$UP_NAME" ]; then + left=0 + [ ! -f "$(state_dir "$RUN_VM")/env" ] || left=unknown + fi leftover "left=$left owner=$OWNER vm=$RUN_VM others=$others" if [ "$left" = 0 ]; then [[ "$SINK_FLUSH_FAILED" != *" $RUN_VM "* ]] || [ "$rc" -ne 0 ] || exit 1 return 0 fi - log "VM cleanup not confirmed for $RUN_VM (left=$left); destroy it with: $0 down $RUN_VM" + [ -z "$UP_NAME" ] || log "VM cleanup not confirmed for $RUN_VM (left=$left); destroy it with: $0 down $RUN_VM" [ "$rc" -ne 0 ] || exit 1 } diff --git a/test/helpers/ubi-runner-fake-vm.ts b/test/helpers/ubi-runner-fake-vm.ts index 35dcac91d..b46f8313c 100644 --- a/test/helpers/ubi-runner-fake-vm.ts +++ b/test/helpers/ubi-runner-fake-vm.ts @@ -17,7 +17,11 @@ export const read = (rel: string) => readFileSync(join(ROOT, rel), 'utf8'); * answer: listed.create-status answers every create with that HTTP status, and * with listed.create-lands (a count N) the VM still lands, listed from the Nth * show on (0: listed at once, as a holder that took the name first), and with listed.show-fails-after-create every later show answers 503; - * listed.list-fails, listed.show-fails and listed.destroy-fails answer 503 or 500; listed.list-body is the list's 200 body. The fake VM is the directory vm/: + * listed.list-fails, listed.show-fails and listed.destroy-fails answer 503 or 500; listed.list-body is the list's 200 body. + * With listed.holder-key (a public key line), the VM at the name is another caller's: the fake ssh + * refuses every key but that one with exit 255, as sshd answers a key the VM was not created with; with listed.agent-holds-holder-key + * the controller's ssh agent holds the holder's key, which ssh offers unless IdentitiesOnly=yes; with listed.config-holds-holder-key + * an ssh config IdentityFile names it, which ssh offers even under IdentitiesOnly=yes unless -F /dev/null. The fake VM is the directory vm/: * the fake ssh records every remote command in ssh.log, keeps copies of * ~/.ubirun-env and ~/.ubirun-setup.sh under the fixture root, serves pulls * from remote-fs/ (a pull's existence check matches only what remote-fs/ holds), and runs every other command there for real (HOME=vm/, @@ -51,6 +55,7 @@ export function fakeVm(opts: { listed?: string; refuse?: string } = {}) { 'root=' + JSON.stringify(root) + '; vm="$root/vm"\n' + 'cmd=${*: -1}\n' + 'printf \'%s\\n\' "$cmd" >> "$root/ssh.log"\n' + + 'if [ -f "$root/listed.holder-key" ]; then k=""; p=""; for a in "$@"; do [ "$p" = -i ] && k=$a; p=$a; done; { [ -f "$root/listed.agent-holds-holder-key" ] && [[ " $* " != *" IdentitiesOnly=yes "* ]]; } || { [ -f "$root/listed.config-holds-holder-key" ] && [[ " $* " != *" -F /dev/null "* ]]; } || { [ -n "$k" ] && [ "$(cut -d" " -f1,2 "$k.pub" 2>/dev/null)" = "$(cut -d" " -f1,2 "$root/listed.holder-key")" ]; } || { echo "ubi@192.0.2.1: Permission denied (publickey)." >&2; exit 255; }; fi\n' + 'export HOME="$vm" PATH="$root/vmbin:$PATH"\n' + 'cd "$vm" || exit 255\n' + 'case "$cmd" in\n' + diff --git a/test/ubicloud-runner-recovery.test.ts b/test/ubicloud-runner-recovery.test.ts index c086046bd..af2a29f6b 100644 --- a/test/ubicloud-runner-recovery.test.ts +++ b/test/ubicloud-runner-recovery.test.ts @@ -399,6 +399,69 @@ describe('ubi-runner money path: an API error is never read as success (GST-1441 } }, 60_000); + // Value: protects=a VM another caller took between our 404 name check and our create is never destroyed when our create's answer (a 409) is lost; fails_when=the by-name lookup after an ambiguous create adopts the name with teardown armed before this call's ssh key opens the VM; why_new=the GST-1441 tests held the name before the check or had the landed VM accept any key; seam=the holder lands with its own key between check and create, and our create answers 000; no_claim=how often a real gateway loses a 409 + test('an ambiguous create whose name another caller took is not this call\'s: its key never opens it, UBI_CREATE_UNKNOWN, nothing destroyed', () => { + const f = make(); + const repo = gitSrc(f); + const other = 'ubirun-thrd1-1000000000-bbbbbbbb-t3'; + for (const [name, args] of [[NAME, ['up', '-n', NAME, '-l', 'eu-central-h1']], [other, ['run', '-n', other, '-l', 'eu-central-h1', '--src', repo, '--', 'true']]] as const) { + writeFileSync(join(f.root, 'listed'), ''); + flag(f, 'create-status', '000'); + flag(f, 'create-lands', '0'); + flag(f, 'holder-key', 'ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIHolderKeyOfAnotherCaller0000000000000000 other-caller\n'); + const r = run(f, [...args], { UBI_RECOVERY_DIR: rd(f), UBI_REAP: '0', UBI_CREATE_GRACE: '5', UBI_SSH_WAIT: '3', UBI_DOWN_TIMEOUT: '2' }); + expect(r.exitCode, out(r)).not.toBe(0); + expect(out(r)).toContain('UBI_CREATE_UNKNOWN'); + expect(f.calls()).not.toContain('"destroy"'); + expect(f.listed()).toContain(name); + expect(recorded(f, name)).toBe(true); + expect(existsSync(bundle(f, name))).toBe(true); + if (args[0] === 'run') expect(out(r)).toContain('UBICLOUD: left=unknown owner=thrd1 vm=' + name); + } + }, 60_000); + + // Value: protects=the ownership proof counts only this call's generated key, so a holder VM that accepts a key from the controller's ssh agent, ~/.ssh or ssh config is never armed and destroyed; fails_when=ssh offers agent, default or config identities beside -i and the proof passes on the holder's key; why_new=the holder-key test's fake offered only the -i key; seam=the fake agent and ssh config hold the holder's key, offered unless IdentitiesOnly=yes and -F /dev/null; no_claim=which keys a real agent holds + test('the ownership proof ignores keys the controller\'s ssh agent or ssh config offers: a holder VM that accepts one is not armed or destroyed', () => { + const f = make(); + const name = 'ubirun-thrd1-1000000000-cccccccc-t3'; + flag(f, 'create-status', '000'); + flag(f, 'create-lands', '0'); + flag(f, 'holder-key', 'ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIHolderKeyOfAnotherCaller0000000000000000 other-caller\n'); + flag(f, 'agent-holds-holder-key', '1'); + flag(f, 'config-holds-holder-key', '1'); + const r = run(f, ['run', '-n', name, '-l', 'eu-central-h1', '--src', gitSrc(f), '--', 'true'], { UBI_RECOVERY_DIR: rd(f), UBI_REAP: '0', UBI_CREATE_GRACE: '5', UBI_SSH_WAIT: '3', UBI_DOWN_TIMEOUT: '2' }); + expect(r.exitCode, out(r)).not.toBe(0); + expect(out(r)).toContain('UBI_CREATE_UNKNOWN'); + expect(f.calls()).not.toContain('"destroy"'); + expect(f.listed()).toContain(name); + }, 60_000); + + // Value: protects=an ambiguous create that landed as this call's own VM is still torn down once its key proves it, so the ownership check never orphans our own VM; fails_when=teardown stays off after the key opens the VM and run leaves it billing; why_new=no test ran a full run on a VM adopted after an ambiguous create; seam=none; no_claim=real ssh timing + test('an ambiguous create that landed as this call\'s own VM arms teardown once its key opens it: run destroys it, left=0', () => { + const f = make(); + flag(f, 'create-status', '000'); + flag(f, 'create-lands', '2'); + const r = run(f, ['run', '--src', gitSrc(f), '--', 'true'], { UBI_RECOVERY_DIR: rd(f), UBI_REAP: '0', UBI_CREATE_GRACE: '20' }); + expect(r.exitCode, out(r)).toBe(0); + expect(out(r)).toContain('answered HTTP 000; looking it up by name'); + expect(f.calls()).toContain('"destroy"'); + expect(out(r)).toMatch(/UBICLOUD: left=0 owner=thrd1 vm=\S+/); + expect(f.listed().trim()).toBe(''); + }, 60_000); + + // Value: protects=a 5xx create whose name stays absent through the grace is settled without any destroy, now that teardown is off after an ambiguous create; fails_when=the absent case leaves a stale record and bundle behind, or sends a destroy for a name this call never proved; why_new=the lookup's 404 branch had no test; seam=none; no_claim=real outage shapes + test('a 5xx create whose name stays absent drops its record and bundle and sends no destroy', () => { + const f = make(); + flag(f, 'create-status', '503'); + const r = run(f, ['up', '-n', NAME, '-l', 'eu-central-h1'], { UBI_RECOVERY_DIR: rd(f), UBI_REAP: '0', UBI_CREATE_GRACE: '2' }); + expect(r.exitCode, out(r)).not.toBe(0); + expect(out(r)).toContain('was still absent 2s after the request'); + expect(f.calls()).not.toContain('"destroy"'); + expect(existsSync(join(f.state, NAME))).toBe(false); + expect(recorded(f, NAME)).toBe(false); + expect(existsSync(bundle(f, NAME))).toBe(false); + }, 60_000); + // Value: protects=a create nobody can confirm either way keeps its record and bundle for the reaper and recovery; fails_when=an unconfirmable 5xx create is settled as refused and its bundle dropped; why_new=no fake create ever failed; seam=none; no_claim=real outage shapes test('a 5xx create the lookup cannot settle is UBI_CREATE_UNKNOWN: record and bundle kept, exit non-zero, nothing destroyed', () => { const f = make(); diff --git a/ubicloud/SKILL.md b/ubicloud/SKILL.md index 5bc7ab60b..53cf66f86 100644 --- a/ubicloud/SKILL.md +++ b/ubicloud/SKILL.md @@ -252,7 +252,9 @@ fail only with `--rerun-failed`, and ends with `UBI_VERIFY: …`. - `UBICLOUD: left= owner= vm= others=` follows teardown. Report it with every result: `left=0` means this run's VM is confirmed gone. `left=1` or `left=unknown` turns a passing command into exit 1; run the printed `down` - command now. `others` counts this owner's other VMs, for information. + command now. When no `down` command is printed (after `UBI_CREATE_UNKNOWN`), + the VM may be another caller's: run `list`, and run `down` only once you know + the VM is yours. `others` counts this owner's other VMs, for information. `run --keep` prints `left=1 ... kept=1` instead and keeps its exit status. - Exit 124 with `watchdog: printed nothing for s` (or `ran s`) means the remote hung or exceeded its budget; the last 40 lines follow. diff --git a/ubicloud/SKILL.md.tmpl b/ubicloud/SKILL.md.tmpl index 2b8aa447a..06893bddf 100644 --- a/ubicloud/SKILL.md.tmpl +++ b/ubicloud/SKILL.md.tmpl @@ -178,7 +178,9 @@ fail only with `--rerun-failed`, and ends with `UBI_VERIFY: …`. - `UBICLOUD: left= owner= vm= others=` follows teardown. Report it with every result: `left=0` means this run's VM is confirmed gone. `left=1` or `left=unknown` turns a passing command into exit 1; run the printed `down` - command now. `others` counts this owner's other VMs, for information. + command now. When no `down` command is printed (after `UBI_CREATE_UNKNOWN`), + the VM may be another caller's: run `list`, and run `down` only once you know + the VM is yours. `others` counts this owner's other VMs, for information. `run --keep` prints `left=1 ... kept=1` instead and keeps its exit status. - Exit 124 with `watchdog: printed nothing for s` (or `ran s`) means the remote hung or exceeded its budget; the last 40 lines follow.