mirror of
https://github.com/garrytan/gstack.git
synced 2026-11-07 17:28:23 +01:00
fix(ubi-runner): an ambiguous create arms teardown only once this call's ssh key opens the VM (GST-1463) (#4046)
* fix(ubi-runner): an ambiguous create arms teardown only once this call's ssh key opens the VM (GST-1463) With an explicit -n name a second caller can take the name between the 404 name check and the create; if our create's 409 was lost (000), the by-name lookup adopted that caller's VM and teardown destroyed it. Teardown is now off from an ambiguous create answer on and armed only when this call's key opens the VM within UBI_SSH_WAIT; otherwise up keeps the record and bundle, exits UBI_CREATE_UNKNOWN and leaves the VM to its TTL, and run reports left=unknown. Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> * fix: ubi-runner ssh proof offers only this call's key (IdentitiesOnly=yes) The ownership proof after an ambiguous create counted any ssh success, and ssh also offered agent and default ~/.ssh keys beside -i, so a holder VM that accepted one of them was armed and destroyed. Failing-first test: the fake agent holds the holder's key. Review-Finding: p1/specialist-batched#1 reader=specialist-batched Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> * fix: ubi-runner ownership proof ignores ssh config identities (-F /dev/null) IdentitiesOnly=yes still offers IdentityFile and CertificateFile entries from ssh config, so the proof after an ambiguous create could pass on a holder VM that accepts a configured key. The unproven proof now runs with -F /dev/null; the normal path keeps the user's config. Failing-first: the fake ssh config holds the holder's key. Review-Finding: p2/red-team#1 reader=red-team Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> * docs: ubicloud skill says to list before down after UBI_CREATE_UNKNOWN left=unknown can now mean another caller's VM; the runner prints list, not down, for that case. The changelog fragment notes that agent and config keys don't count for the proof. Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --------- Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com>
This commit is contained in:
Garry Tanandcapy-ai[bot] authored and GitHub committed
1 parent
4c400661d9
commit
065afdfdcf
7 files changed
+105
-16
No files matched your search
@@ -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)
|
||||
@@ -7055,7 +7055,7 @@ These codes come from `scripts/ubicloud/ubi-runner.sh` (GST-1220). `RECOVERED`,
|
||||
<a id="ubi-create-unknown"></a>
|
||||
### `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 <name>` command to find out, then rerun. This owner's reap destroys an unconfirmed VM once its `-t<hours>` 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 <name>` 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<hours>` TTL ends.
|
||||
|
||||
<a id="ubi-attach-unknown"></a>
|
||||
### `UBI_ATTACH_UNKNOWN`
|
||||
|
||||
@@ -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<hours> 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<hours> 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<hours> 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
|
||||
}
|
||||
|
||||
|
||||
@@ -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' +
|
||||
|
||||
@@ -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();
|
||||
|
||||
+3
-1
@@ -252,7 +252,9 @@ fail only with `--rerun-failed`, and ends with `UBI_VERIFY: …`.
|
||||
- `UBICLOUD: left=<n> owner=<o> vm=<name> others=<n>` 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: <phase> printed nothing for <n>s` (or `ran <n>s`)
|
||||
means the remote hung or exceeded its budget; the last 40 lines follow.
|
||||
|
||||
@@ -178,7 +178,9 @@ fail only with `--rerun-failed`, and ends with `UBI_VERIFY: …`.
|
||||
- `UBICLOUD: left=<n> owner=<o> vm=<name> others=<n>` 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: <phase> printed nothing for <n>s` (or `ran <n>s`)
|
||||
means the remote hung or exceeded its budget; the last 40 lines follow.
|
||||
|
||||
Reference in new issue
Block a user