diff --git a/container/lib/common.sh b/container/lib/common.sh index e17e577..add5e17 100644 --- a/container/lib/common.sh +++ b/container/lib/common.sh @@ -71,15 +71,13 @@ emit() { || warn "callback '$event' failed (ignored)" } -# Resolve one of a target pair under manifest .upload. to a concrete URL: -# a fixed , or a whose {name} is the file's -# basename. Both pairs in the manifest are shaped this way, so the same -# resolution serves the direct target and the signing endpoint. -# upload_target -upload_target() { +# Resolve the signing endpoint for one artifact under manifest .upload.: +# a fixed .signUrl, or a .signTemplate whose {name} is the file's basename. +# signing_endpoint +signing_endpoint() { local key="$1" file="$2" url template - url="$(mq ".upload.\"$key\".$3")" - template="$(mq ".upload.\"$key\".$4")" + url="$(mq ".upload.\"$key\".signUrl")" + template="$(mq ".upload.\"$key\".signTemplate")" if [ -z "$url" ] && [ -n "$template" ]; then url="${template//\{name\}/$(basename "$file")}" fi @@ -101,16 +99,17 @@ mint_upload_url() { } # upload -# upload-key indexes manifest .upload., which carries a direct target -# (.url or .urlTemplate) and, preferably, a signing endpoint (.signUrl or -# .signTemplate). +# upload-key indexes manifest .upload., which carries a signing endpoint +# (.signUrl or .signTemplate). That is the only route: arena mints a presigned +# PUT and sends the bytes to S3 itself. # -# The signed path sends the bytes to S3 and is the one to want. The direct -# target reads the whole artifact into headquarters' memory to re-send it, on a -# t4g.nano that also carries the API and Caddy - and a match's screenshots, +# There used to be a fallback that PUT the artifact to headquarters' own API and +# let it re-send the bytes. It is gone. It read whole artifacts into memory on a +# t4g.nano that also carries the API and Caddy, and a match's screenshots, # summary GIF, profiles and logs all arrive within a few seconds of each other -# at the end. It stays as the fallback so a minting failure costs a round trip -# rather than the artifact. +# at the end. Keeping it as a fallback meant a broken signing setup degraded to +# the slow route silently instead of saying so. Now a minting failure fails the +# upload: the artifact stays in the container and the log says which key. # # The signature is minted here, immediately before use, so its life never has to # cover the length of a match. That is not a detail: a presigned URL cannot @@ -119,26 +118,16 @@ mint_upload_url() { # will not issue a fresh set on request. Signed at launch, every upload URL # would expire on a clock nobody can predict. upload() { - local file="$1" key="$2" method target signer signed - method="$(mq ".upload.\"$key\".method" PUT)" - target="$(upload_target "$key" "$file" url urlTemplate)" - signer="$(upload_target "$key" "$file" signUrl signTemplate)" + local file="$1" key="$2" signer target + signer="$(signing_endpoint "$key" "$file")" + [ -n "$signer" ] || { log "no upload target for '$key'; keeping $file locally"; return 1; } - if [ -n "$signer" ]; then - if signed="$(mint_upload_url "$signer")" && [ -n "$signed" ]; then - target="$signed" - # The signature covers the method, so it is PUT whatever the manifest - # said for the direct target. - method=PUT - else - warn "could not mint an upload URL for '$key'; sending it through the API instead" - fi - fi - - [ -n "$target" ] || { log "no upload target for '$key'; keeping $file locally"; return 1; } + target="$(mint_upload_url "$signer")" && [ -n "$target" ] \ + || { warn "could not mint an upload URL for '$key'; keeping $file locally"; return 1; } + # PUT, not the manifest's .method: the signature covers the method. curl -fsS --retry 3 --retry-delay 2 --max-time "${ARENA_UPLOAD_TIMEOUT:-120}" \ - -X "$method" --upload-file "$file" "$target" >/dev/null \ + -X PUT --upload-file "$file" "$target" >/dev/null \ || { warn "upload of $(basename "$file") to '$key' failed"; return 1; } log "uploaded $(basename "$file") -> $key" } diff --git a/tests/shell/test-upload.sh b/tests/shell/test-upload.sh index f99ffdd..d8a9dd9 100644 --- a/tests/shell/test-upload.sh +++ b/tests/shell/test-upload.sh @@ -1,9 +1,10 @@ #!/usr/bin/env bash -# upload() has two paths now: ask headquarters to sign a PUT and send the bytes -# straight to S3, or PUT them through headquarters itself. Which one a manifest -# gets is worth pinning down, because the two failures here are both invisible -# from outside - quietly taking the slow path that buffers whole artifacts in a -# 512MB instance, and losing an artifact when signing fails. +# upload() has one path: ask headquarters to sign a PUT and send the bytes +# straight to S3. There is no fallback through headquarters' own API any more, +# so what these cases pin down is that the direct route stays unused - including +# when a manifest still offers one, and when signing fails. Buffering whole +# artifacts in a 512MB instance was the thing being removed, and it would come +# back invisibly. # # tests/upload-stub.py plays both ends, so this needs no AWS and no network. set -uo pipefail @@ -47,7 +48,9 @@ run_upload() { # both_targets="{\"diagnostics\":{\"method\":\"PUT\",\ \"urlTemplate\":\"$BASE/api/{name}\",\"signTemplate\":\"$BASE/sign/{name}\"}}" -# --- a manifest offering both: the signed path wins ------------------------- +# --- a manifest still offering a direct target: it is ignored --------------- +# This is the deploy order arena has to survive: it ships before headquarters +# drops urlTemplate, so both fields arrive and only the signing one is read. manifest "$both_targets" signed_out="$(run_upload diagnostics)"; status=$? @@ -56,7 +59,8 @@ check "the signing endpoint was asked for this artifact by name" \ grep -qx 'POST /sign/megamek.log' "$TMP/requests" check "the bytes went to the signed destination" \ grep -qx 'PUT /s3/megamek.log' "$TMP/requests" -# The whole point of the change: nothing reaches the API's own route. +# The whole point of the change: nothing reaches the API's own route, even +# though this manifest hands it one. check "and not through the API" \ bash -c "! grep -q '^PUT /api/' '$TMP/requests'" # A minted URL is a bearer credential for that key. common.sh's own rules say @@ -65,25 +69,34 @@ check "and not through the API" \ check "the signed URL never reaches the log" \ bash -c "! grep -q '/s3/' <<<\"\$1\"" _ "$signed_out" -# --- minting fails: fall back rather than lose the artifact ----------------- +# --- minting fails: the upload fails with it -------------------------------- +# It used to fall back to the API. Losing the artifact is the deliberate cost of +# not having a slow route that can come back unnoticed - and the failure is in +# the log, where reverting to the API was not. echo fail > "$TMP/mint-mode" : > "$TMP/requests" out="$(run_upload diagnostics)"; status=$? -check "a minting failure still uploads" test "$status" -eq 0 -check "by sending it through the API instead" \ - grep -qx 'PUT /api/megamek.log' "$TMP/requests" -check "and says which key fell back" grep -q "could not mint an upload URL for 'diagnostics'" <<<"$out" - -# --- a manifest from before signing existed -------------------------------- -# headquarters can be older than the container. Nothing should try to mint. +check "a minting failure fails the upload" test "$status" -ne 0 +check "and says which key it was for" grep -q "could not mint an upload URL for 'diagnostics'" <<<"$out" +check "and nothing is sent through the API" \ + bash -c "! grep -q '^PUT ' '$TMP/requests'" +check "with nothing claiming the file was uploaded" \ + bash -c "! grep -q 'uploaded megamek.log' <<<\"\$1\"" _ "$out" + +# --- a manifest with a direct target and nothing to sign with --------------- +# What an older headquarters sends. There is no route for it now, so the file is +# kept rather than pushed through the API. 10-manifest.sh refuses such a +# manifest at container start; this is upload() holding the line anyway. echo ok > "$TMP/mint-mode" : > "$TMP/requests" manifest "{\"diagnostics\":{\"method\":\"PUT\",\"urlTemplate\":\"$BASE/api/{name}\"}}" out="$(run_upload diagnostics)"; status=$? -check "a manifest with no signing target still uploads" test "$status" -eq 0 +check "a manifest with no signing target uploads nothing" test "$status" -ne 0 check "and does not try to mint" bash -c "! grep -q '^POST ' '$TMP/requests'" +check "and does not fall back to the direct target it was given" \ + test ! -s "$TMP/requests" # --- no target at all ------------------------------------------------------- # The state every diagnostics artifact was in before headquarters carried the diff --git a/tests/upload-stub.py b/tests/upload-stub.py index 6e4001e..8cb3bf1 100644 --- a/tests/upload-stub.py +++ b/tests/upload-stub.py @@ -4,12 +4,13 @@ POST /sign/ mints a URL pointing at this server's /s3/, or fails, per the first line of /mint-mode PUT /s3/ the signed destination - PUT /api/ the direct fallback, standing in for headquarters + PUT /api/ headquarters' own route, which arena no longer uses Every request is appended to /requests as "METHOD path", which is what the -shell test asserts on: the question is always which of the two ends received the -bytes. The port is written to /port once bound, because the test asks for -port 0 rather than guessing at a free one. +shell test asserts on. /api/ is still served so the test can show that nothing +arrives there, even from a manifest that offers it. The port is written to +/port once bound, because the test asks for port 0 rather than guessing at +a free one. """ import http.server