From 6b5ea8d8161f71d2f6c56dd509a4fbe3d0fcdbae Mon Sep 17 00:00:00 2001 From: Tom Elliott Date: Wed, 26 Aug 2026 12:19:00 -0500 Subject: [PATCH] Report why a server call failed everywhere, not just at task completion fos#162 gave fog.imgcomplete and fog.nonimgcomplete a helper that tells a transport failure, an HTTP error, an empty 2xx and a real answer apart. Every other script that talks to the server still read `res=$(curl -Lks --data ...)`, which collapses all four into one empty string -- so the same blank failure fos#162 fixed at completion was still there at check-in, registration, inventory, capone and the multicast capability probe. postToServer is generalized to callServer [data]: GET when no data is given, POST when there is, since it is the same exchange either way. It returns 0 with a body, 1 when the call failed, and 2 when the server answered 2xx with nothing -- three outcomes rather than two, because a few endpoints answer empty on purpose and a caller must be able to tell that from a dead server. That is what fog.man.reg's location and OU prompts need. 23 call sites across 12 scripts are converted. Genuine bugs found on the way: fog.inventory printed "Done" after giving up on all eleven attempts. fog.capone reported "Count not find" and blamed the image for what could be a transport failure. restoreLVMPartition's multicast capability probe told the operator their server was too old to multicast LVM when the probe had never reached it -- a claim about the server's VERSION drawn from a dead network. fog.man.reg's locationcheck/oucheck skipped their prompts silently when the GET failed, so a host registered into no location with no sign anything had gone wrong. fog.checkin and fog.man.reg looped without reporting anything at all. Also fixes a regression shipped in fos#162: `res=$(curl ...)` strips trailing newlines and the helper did not, so a server whose reply ended in a newline yielded "##\n", every `== "##"` comparison failed, and a capture that had SUCCEEDED retried eleven times and gave up. Test 5b pins it -- and had to be written twice, because the first cut read the body through `head -1`, which cannot see a trailing newline and stayed green with the fix removed. Left on raw curl deliberately, each with its reason at the call site: fog.statusreporter (a 3s ping would paint over imaging output), fog.av (fire-and-forget), reportToServer (its contract is silence, pinned by tests/checks/error-report.sh), the secureboot-funcs.sh and bin/fog `-o` downloads, and S40network's connectivity probe. tests/checks/server-post-reporting.sh grows to 15 assertions: the GET/POST split, the three return codes, and two whole-file sweeps -- no call site anywhere in the overlay may wrap callServer in $( ), and raw curl may read a reply only in the exceptions named above. tests/checks/lvm.sh gains case 30 for the capability probe, and its curl stub now emits the -w status line it always implied. Every new assertion was mutation-verified red before being kept. Co-Authored-By: Claude --- .../board/FOG/FOS/rootfs_overlay/bin/fog | 10 +- .../FOG/FOS/rootfs_overlay/bin/fog.auto.reg | 7 +- .../board/FOG/FOS/rootfs_overlay/bin/fog.av | 2 + .../FOG/FOS/rootfs_overlay/bin/fog.capone | 8 +- .../FOG/FOS/rootfs_overlay/bin/fog.checkin | 10 +- .../FOG/FOS/rootfs_overlay/bin/fog.checkmount | 8 +- .../FOS/rootfs_overlay/bin/fog.imgcomplete | 2 +- .../FOG/FOS/rootfs_overlay/bin/fog.inventory | 20 ++- .../FOG/FOS/rootfs_overlay/bin/fog.man.reg | 45 ++++-- .../FOS/rootfs_overlay/bin/fog.nonimgcomplete | 2 +- .../FOS/rootfs_overlay/bin/fog.statusreporter | 5 + .../FOS/rootfs_overlay/bin/fog.surfacetest | 5 +- .../rootfs_overlay/usr/share/fog/lib/funcs.sh | 99 +++++++++----- tests/README.md | 34 +++-- tests/checks/lvm.sh | 35 ++++- tests/checks/server-post-reporting.sh | 129 ++++++++++++++++-- 16 files changed, 333 insertions(+), 88 deletions(-) diff --git a/Buildroot/board/FOG/FOS/rootfs_overlay/bin/fog b/Buildroot/board/FOG/FOS/rootfs_overlay/bin/fog index 711cedc1..6b03a4e1 100755 --- a/Buildroot/board/FOG/FOS/rootfs_overlay/bin/fog +++ b/Buildroot/board/FOG/FOS/rootfs_overlay/bin/fog @@ -6,7 +6,15 @@ if [[ $boottype == usb && ! -z $web ]]; then sysuuid=${sysuuid,,} mac=$(getMACAddresses) base64mac=$(echo $mac | base64) - token=$(curl -Lks --data "mac=$base64mac" "${web}status/hostgetkey.php") + callServer "${web}status/hostgetkey.php" "mac=$base64mac" + token="$serverBody" + # Said nothing at all when the key could not be fetched. The next call then + # posts an empty hosttoken, and whatever comes back is written to + # /tmp/hinfo.txt and SOURCED -- so a failure here surfaced much later, as + # missing kernel arguments rather than as a failed request. + [[ -z $token ]] && echo " * Could not get this host's key: ${serverReason:-the server returned no key}" + # Left as a raw curl deliberately: -o writes a file rather than returning a + # body, which is not what callServer is for. curl -Lks -o /tmp/hinfo.txt --data "sysuuid=${sysuuid}&mac=$mac&hosttoken=${token}" "${web}service/hostinfo.php" -A '' [[ -f /tmp/hinfo.txt ]] && . /tmp/hinfo.txt fi diff --git a/Buildroot/board/FOG/FOS/rootfs_overlay/bin/fog.auto.reg b/Buildroot/board/FOG/FOS/rootfs_overlay/bin/fog.auto.reg index 2f9b65e2..3ec92646 100755 --- a/Buildroot/board/FOG/FOS/rootfs_overlay/bin/fog.auto.reg +++ b/Buildroot/board/FOG/FOS/rootfs_overlay/bin/fog.auto.reg @@ -73,7 +73,8 @@ if [[ -f /sys/firmware/acpi/tables/MSDM ]]; then productKey=$(tail -c+57 /sys/firmware/acpi/tables/MSDM | base64) fi while [[ -z $res ]]; do - res=$(curl -Lks --data "sysserial=${sysserial}&sysuuid=${sysuuid}&mac=$mac&productKey=${productKey}" ${web}service/auto.register.php 2>/dev/null) + callServer "${web}service/auto.register.php" "sysserial=${sysserial}&sysuuid=${sysuuid}&mac=$mac&productKey=${productKey}" + res="$serverBody" case $count in [0-8]) let count+=1 @@ -82,7 +83,9 @@ while [[ -z $res ]]; do 9) echo "Failed" debugPause - handleError "Cannot register host. ($0)\n Args Passed: $*" + # Was "Cannot register host." and nothing else, for every cause: + # server down, server refusing, server answering with nothing. + handleError "Cannot register host: ${serverReason:-no reason reported} ($0)\n Args Passed: $*" ;; esac done diff --git a/Buildroot/board/FOG/FOS/rootfs_overlay/bin/fog.av b/Buildroot/board/FOG/FOS/rootfs_overlay/bin/fog.av index 9ffce61e..99c1151f 100755 --- a/Buildroot/board/FOG/FOS/rootfs_overlay/bin/fog.av +++ b/Buildroot/board/FOG/FOS/rootfs_overlay/bin/fog.av @@ -169,6 +169,8 @@ for disk in $disks; do let cnt+=1 echo " * File id: $cnt" dots "Sending file id" + # Fire and forget: nothing reads a reply, so there is no reply to + # misreport and callServer would add nothing. curl -Lks --data "sysuuid=${sysuuid}&mac=$mac&string=$b64&mode=$avmode" ${web}service/av.php 2>/dev/null case $? in 0) diff --git a/Buildroot/board/FOG/FOS/rootfs_overlay/bin/fog.capone b/Buildroot/board/FOG/FOS/rootfs_overlay/bin/fog.capone index 86c59531..a33f6fcf 100755 --- a/Buildroot/board/FOG/FOS/rootfs_overlay/bin/fog.capone +++ b/Buildroot/board/FOG/FOS/rootfs_overlay/bin/fog.capone @@ -23,12 +23,16 @@ dots "Looking for images" count=0 imgret="" while [[ -z $imgret ]]; do - imgret=$(curl -Lks --data "action=imagelookup&key=${dmi64}" ${web}service/capone.php 2>/dev/null) + callServer "${web}service/capone.php" "action=imagelookup&key=${dmi64}" + imgret="$serverBody" [[ -n $imgret ]] && continue if [[ $count -ge 10 ]]; then echo "Failed" debugPause - handleError "Count not find an Image definition ($0)\n Args Passed: $*" + # "Count not" was a typo for "Cannot", and the message blamed a + # missing image definition for what is just as often a server that + # could not be reached -- the reply is empty either way. + handleError "Cannot find an Image definition: ${serverReason:-the server returned no image for this machine} ($0)\n Args Passed: $*" fi let count+=1 usleep 5000000 diff --git a/Buildroot/board/FOG/FOS/rootfs_overlay/bin/fog.checkin b/Buildroot/board/FOG/FOS/rootfs_overlay/bin/fog.checkin index 739de2e9..713c4ece 100755 --- a/Buildroot/board/FOG/FOS/rootfs_overlay/bin/fog.checkin +++ b/Buildroot/board/FOG/FOS/rootfs_overlay/bin/fog.checkin @@ -54,12 +54,18 @@ checkin() { local count=0 local res="" local waittime=0 + # No bail-out, and that is deliberate: a host waiting its turn in a queue + # must keep waiting however long that takes. What was NOT deliberate is that + # a dead server looked identical to a queue position -- $res was empty, so + # this printed " * (In line for 5s)" forever with nothing to diagnose. + # serverReason distinguishes them; the waiting behaviour is unchanged. while [[ $res != "##@GO" ]]; do - res=$(curl -Lks --data "$poststring" ${web}service/$php_post 2>/dev/null) + callServer "${web}service/$php_post" "$poststring" + res="$serverBody" if [[ $res != "##@GO" ]]; then echo "Failed" debugPause - echo -n " * $res (In line for " + echo -n " * ${serverReason:-$res} (In line for " sec2string "$waittime" echo ")" let waittime+=5 diff --git a/Buildroot/board/FOG/FOS/rootfs_overlay/bin/fog.checkmount b/Buildroot/board/FOG/FOS/rootfs_overlay/bin/fog.checkmount index 9e079702..77759a93 100755 --- a/Buildroot/board/FOG/FOS/rootfs_overlay/bin/fog.checkmount +++ b/Buildroot/board/FOG/FOS/rootfs_overlay/bin/fog.checkmount @@ -5,7 +5,11 @@ dots "Checking Mounted File System" if [[ ! -f /images/.mntcheck ]]; then count=0 while [[ $blame != '##' ]]; do - blame=$(curl -Lks --data "sysuuid=${sysuuid}&mac=$mac&type=$type" ${web}service/blame.php 2>/dev/null) + # $blame was reported verbatim in the handleError below, so a server + # that answered with nothing produced "Error during failure + # notification: " -- an error message about an error, carrying neither. + callServer "${web}service/blame.php" "sysuuid=${sysuuid}&mac=$mac&type=$type" + blame="${serverBody:-$serverReason}" case $count in [0-8]) let count+=1 @@ -14,7 +18,7 @@ if [[ ! -f /images/.mntcheck ]]; then 9) echo "Failed" debugPause - handleError "Error during failure notification: $blame ($0)\n Args Passed: $*" + handleError "Error during failure notification: ${serverReason:-$blame} ($0)\n Args Passed: $*" ;; esac done diff --git a/Buildroot/board/FOG/FOS/rootfs_overlay/bin/fog.imgcomplete b/Buildroot/board/FOG/FOS/rootfs_overlay/bin/fog.imgcomplete index 8a52466b..f2252190 100755 --- a/Buildroot/board/FOG/FOS/rootfs_overlay/bin/fog.imgcomplete +++ b/Buildroot/board/FOG/FOS/rootfs_overlay/bin/fog.imgcomplete @@ -23,7 +23,7 @@ while [[ $res != "##" ]]; do # a subshell would discard both. This used to print $res as the error, so an # empty reply was reported as an empty error and there was nothing to # diagnose from here -- see fogproject GH-1380. - postToServer "${web}service/$php_post" "$poststring" + callServer "${web}service/$php_post" "$poststring" res="$serverBody" [[ $res == "##" ]] && break echo "Failed" diff --git a/Buildroot/board/FOG/FOS/rootfs_overlay/bin/fog.inventory b/Buildroot/board/FOG/FOS/rootfs_overlay/bin/fog.inventory index 6ab6d3a8..22980116 100755 --- a/Buildroot/board/FOG/FOS/rootfs_overlay/bin/fog.inventory +++ b/Buildroot/board/FOG/FOS/rootfs_overlay/bin/fog.inventory @@ -97,12 +97,19 @@ fi poststring="mac=${mac}&sysman=${sysman64}&sysproduct=${sysproduct64}&sysversion=${sysversion64}&sysserial=${sysserial64}&sysuuid=${sysuuid64}&systype=${systype64}&biosversion=${biosversion64}&biosvendor=${biosvendor64}&biosdate=${biosdate64}&mbman=${mbman64}&mbproductname=${mbproductname64}&mbversion=${mbversion64}&mbserial=${mbserial64}&mbasset=${mbasset64}&cpuman=${cpuman64}&cpuversion=${cpuversion64}&cpucurrent=${cpucurrent64}&cpumax=${cpumax64}&mem=${mem64}&hdinfo=${hdinfo64}&caseman=${caseman64}&casever=${casever64}&caseserial=${caseserial64}&caseasset=${caseasset64}&gpuvendors=${inventory_graphics_vendor64}&gpuproducts=${inventory_graphics_product64}" count=0 res="" +sent=0 while [[ -z $res ]]; do dots "Attempting to send inventory" - res=$(curl -Lks --data "$poststring" ${web}service/inventory.php 2>/dev/null) + callServer "${web}service/inventory.php" "$poststring" + res="$serverBody" if [[ $count -ge 10 ]]; then echo "Failed" debugPause + # Says why it gave up. It used to break in silence, and the + # unconditional "Done" below then reported the inventory as sent when + # eleven attempts had just failed. + echo " * Inventory was not recorded: ${serverReason:-no reply from the server}" + debugPause break fi if [[ -z $res ]]; then @@ -110,6 +117,15 @@ while [[ -z $res ]]; do usleep 2000000 fi done -echo "Done" +# Gated on the operation, not printed regardless. Inventory is not fatal -- a +# deploy carries on without it, which is why this breaks rather than +# handleError-ing -- but reporting success for something that did not happen is +# how a missing inventory goes unnoticed until someone looks for the host. +[[ -n $res ]] && sent=1 +if [[ $sent -eq 1 ]]; then + echo "Done" +else + echo " * Continuing without inventory" +fi debugPause [[ $deployed -eq 1 ]] && . /bin/fog.nonimgcomplete $mac diff --git a/Buildroot/board/FOG/FOS/rootfs_overlay/bin/fog.man.reg b/Buildroot/board/FOG/FOS/rootfs_overlay/bin/fog.man.reg index 2f048aba..d29594dd 100755 --- a/Buildroot/board/FOG/FOS/rootfs_overlay/bin/fog.man.reg +++ b/Buildroot/board/FOG/FOS/rootfs_overlay/bin/fog.man.reg @@ -61,7 +61,8 @@ mac=$(getMACAddresses | base64) sysuuid=$(dmidecode -s system-uuid) sysuuid=${sysuuid,,} sysuuid=$(echo $sysuuid | base64) -exists=$(curl -Lks --data "sysuuid=${sysuuid}&mac=$mac" ${web}service/man.hostexists.php 2>/dev/null) +callServer "${web}service/man.hostexists.php" "sysuuid=${sysuuid}&mac=$mac" +exists="$serverBody" checkAndSet() { local testvar="$1" local onlynum="$2" @@ -92,7 +93,15 @@ setIDs() { [?]) url="${web}service/${str}listing.php" clearScreen - res=$(echo -e $(curl -ks $url 2>/dev/null)) + # Unquoted $serverBody, as the original's unquoted $( ) was: + # the word splitting is what the loop below iterates on. + # NOTE: this call now follows redirects (callServer uses -L, + # this site used bare -ks). That is a deliberate improvement -- + # a server that redirects HTTP to HTTPS previously returned the + # redirect page here and the listing came out as noise. + callServer "$url" + res=$(echo -e $serverBody) + [[ -z $res ]] && res="${serverReason:-no entries returned}" i=0 IFS=$'\n' for line in $res; do @@ -166,12 +175,22 @@ while [[ $res != "#!ok" ]]; do usleep 2000000 fi host=$(echo $host | base64) - res=$(curl -Lks --data "host=$host" ${web}service/hostnameloop.php 2>/dev/null) - [[ $res != "#!ok" ]] && echo "$res" + callServer "${web}service/hostnameloop.php" "host=$host" + res="$serverBody" + # ${serverReason:-$res}: a failed call printed a blank line and asked for the + # hostname again, forever, with no hint that the server was the problem. + [[ $res != "#!ok" ]] && echo "${serverReason:-$res}" done imageid="" setIDs "imageid" "image" "" "" 20 -if [[ $(curl -Lks ${web}service/locationcheck.php 2>/dev/null) == "##" ]]; then +# The prompt is offered only when the server says "##". A failed call is not +# "##" either, so this used to skip the question in silence and register the +# host with no location. Return 2 (answered, empty body) is a legitimate "no +# locations configured" here and stays quiet; only a real failure is reported, +# and the default is unchanged so the flow is not blocked by a warning. +callServer "${web}service/locationcheck.php" +[[ $? -eq 1 ]] && echo " * Could not ask the server about locations: ${serverReason}" +if [[ $serverBody == "##" ]]; then while [[ -z $askme ]]; do echo -n " Would you like to assign a location for this host? (y/N) " read askme @@ -191,7 +210,9 @@ if [[ $(curl -Lks ${web}service/locationcheck.php 2>/dev/null) == "##" ]]; then done fi askme="" -if [[ $(curl -Lks ${web}service/oucheck.php 2>/dev/null) == "##" ]]; then +callServer "${web}service/oucheck.php" +[[ $? -eq 1 ]] && echo " * Could not ask the server about OUs: ${serverReason}" +if [[ $serverBody == "##" ]]; then while [[ -z $askme ]]; do echo -n " Would you like to assign an ou for this host? (y/N) " read askme @@ -313,7 +334,8 @@ while [[ -z $askme ]]; do read -s password user64=$(echo $username | tr -d '\012' | base64) pass64=$(echo $password | tr -d '\012' | base64) - ret=$(curl -Lks --data "sysuuid=${sysuuid}&mac=$mac&username=$user64&password=$pass64" ${web}service/checkcredentials.php 2>/dev/null) + callServer "${web}service/checkcredentials.php" "sysuuid=${sysuuid}&mac=$mac&username=$user64&password=$pass64" + ret="${serverBody:-$serverReason}" case $ret in '#!ok') echo @@ -355,9 +377,14 @@ while [[ -z $askme ]]; do done dots "Attempting to register host" res="" +# Unbounded on purpose -- registration is the whole point of this screen and +# there is a human present to abort -- but it used to echo an empty line every +# two seconds when the server was unreachable, so the operator watched a blank +# console with no idea anything was wrong. while [[ -z $res ]]; do - res=$(curl -Lks --data "sysuuid=${sysuuid}&mac=$mac&advanced=$(echo -n 1 | base64)&host=$host&imageid=$imageid&primaryuser=$primaryuser&other1=$other1&other2=$other2&doimage=$realdoimage&doad=$blDoAD&location=$location64&ou=$ou64&username=$user64&password=$pass64&groupid=$group64&snapinid=$snapin64&productKey=$productKey" ${web}service/auto.register.php 2>/dev/null) - echo "$res" + callServer "${web}service/auto.register.php" "sysuuid=${sysuuid}&mac=$mac&advanced=$(echo -n 1 | base64)&host=$host&imageid=$imageid&primaryuser=$primaryuser&other1=$other1&other2=$other2&doimage=$realdoimage&doad=$blDoAD&location=$location64&ou=$ou64&username=$user64&password=$pass64&groupid=$group64&snapinid=$snapin64&productKey=$productKey" + res="$serverBody" + echo "${serverReason:-$res}" usleep 2000000 done . /bin/fog.inventory diff --git a/Buildroot/board/FOG/FOS/rootfs_overlay/bin/fog.nonimgcomplete b/Buildroot/board/FOG/FOS/rootfs_overlay/bin/fog.nonimgcomplete index c808882a..a9d15490 100755 --- a/Buildroot/board/FOG/FOS/rootfs_overlay/bin/fog.nonimgcomplete +++ b/Buildroot/board/FOG/FOS/rootfs_overlay/bin/fog.nonimgcomplete @@ -12,7 +12,7 @@ while [[ $res != "##" ]]; do # not even the empty "Error returned:" its sibling managed -- so eleven # attempts could go by with no clue why. Same reporting as fog.imgcomplete # now; see fogproject GH-1380. - postToServer "${web}service/Post_Wipe.php" "sysuuid=${sysuuid}&mac=$mactosend" + callServer "${web}service/Post_Wipe.php" "sysuuid=${sysuuid}&mac=$mactosend" res="$serverBody" [[ $res == "##" ]] && break echo "Failed" diff --git a/Buildroot/board/FOG/FOS/rootfs_overlay/bin/fog.statusreporter b/Buildroot/board/FOG/FOS/rootfs_overlay/bin/fog.statusreporter index 4fb10414..a43f425e 100755 --- a/Buildroot/board/FOG/FOS/rootfs_overlay/bin/fog.statusreporter +++ b/Buildroot/board/FOG/FOS/rootfs_overlay/bin/fog.statusreporter @@ -12,6 +12,11 @@ while :; do cat /dev/null > $tmpfile 2>/dev/null [[ -z $mac ]] && continue status=$(echo $status | base64) + # Deliberately NOT callServer, and $res is deliberately never read. This is + # a best-effort progress ping every three seconds for the life of the task; + # reporting each failure would paint the console over the top of the imaging + # output it exists to report on, and a missed progress update costs nothing. + # The completion scripts are where a failure actually matters. res=$(curl -Lks --data "sysuuid=${sysuuid}&mac=$mac&status=$status" ${web}service/progress.php 2>/dev/null) usleep 3000000 done diff --git a/Buildroot/board/FOG/FOS/rootfs_overlay/bin/fog.surfacetest b/Buildroot/board/FOG/FOS/rootfs_overlay/bin/fog.surfacetest index 5c1c8570..646a5622 100755 --- a/Buildroot/board/FOG/FOS/rootfs_overlay/bin/fog.surfacetest +++ b/Buildroot/board/FOG/FOS/rootfs_overlay/bin/fog.surfacetest @@ -20,11 +20,12 @@ count=0 sysuuid=$(dmidecode -s system-uuid) sysuuid=${sysuuid,,} while [[ $res != "##@GO" ]]; do - res=$(curl -Lks --data "sysuuid=${sysuuid}&mac=$mac" ${web}service/Pre_Stage1.php 2>/dev/null) + callServer "${web}service/Pre_Stage1.php" "sysuuid=${sysuuid}&mac=$mac" + res="$serverBody" if [[ $count -ge 10 ]]; then echo "Failed" debugPause - handleError "Could not checkin. ($0)\n Args Passed: $*" + handleError "Could not checkin: ${serverReason:-last reply was \"$res\"} ($0)\n Args Passed: $*" fi let count+=1 usleep 5000000 diff --git a/Buildroot/board/FOG/FOS/rootfs_overlay/usr/share/fog/lib/funcs.sh b/Buildroot/board/FOG/FOS/rootfs_overlay/usr/share/fog/lib/funcs.sh index 404d0a4b..9d7a7bcc 100644 --- a/Buildroot/board/FOG/FOS/rootfs_overlay/usr/share/fog/lib/funcs.sh +++ b/Buildroot/board/FOG/FOS/rootfs_overlay/usr/share/fog/lib/funcs.sh @@ -26,7 +26,11 @@ clearScreen() { } # Displays the nice banner along with the running version displayBanner() { - version=$(curl -Lks ${web}service/getversion.php 2>/dev/null) + # Cosmetic -- the banner just shows a blank version when this fails, and a + # banner is no place to raise an error -- but it goes through callServer so + # there is exactly one way FOS talks to the server. + callServer "${web}service/getversion.php" + version="$serverBody" echo " ==================================" echo " === ==== ===== ====" echo " === ========= == === == ===" @@ -1653,52 +1657,57 @@ reportToServer() { "${web}service/taskerror.php" &>/dev/null || : return 0 } -# POSTs to a FOG service endpoint and says what actually came back. +# Calls a FOG service endpoint and says what actually came back. # -# The completion scripts wait for a literal "##" and print whatever they got -# when it does not arrive. That reported an EMPTY string as the error whenever -# the server died mid-request, so "Error returned:" was followed by nothing at -# all and there was nothing to diagnose from the client -- which is exactly how -# fogproject GH-1380 presented: a capture that had already been renamed into -# place retried until it gave up, while the server was taking an uncatchable -# PHP memory-exhaustion fatal on every attempt. +# Every FOS script used to do this by hand: # -# Four outcomes have to be told apart, and used to look identical: +# res=$(curl -Lks --data "$poststring" ${web}service/whatever.php 2>/dev/null) # -# curl could not connect - transport, nothing reached the server -# HTTP >= 400 - it answered, and refused -# HTTP 2xx with an empty body - it accepted and returned nothing, which for -# PHP means a fatal error, and the reason is in -# the web server's PHP error log, NOT here -# HTTP 2xx with a body - a real message; print it +# which collapses four very different outcomes into one empty string: curl could +# not connect, the server refused, the server accepted and returned nothing, or +# the server genuinely replied with nothing. Callers then reported the empty +# string as the error, so the console said "Error returned:" with nothing after +# it -- or, more often, said nothing at all and retried until it gave up. That +# is how fogproject GH-1380 presented, and the same shape was in every other +# call site (GH-1380 follow-up sweep). # -# Sets three variables and returns 0 only when the exchange produced a body: +# Sets three variables and answers with THREE outcomes, because "the call +# failed" and "the call worked and the answer was empty" are different questions +# and some callers legitimately treat an empty answer as a no: # -# serverBody - the response body, empty if there was none -# serverStatus - the HTTP status, 000 when curl never connected -# serverReason - a human-readable description of what went wrong, empty on -# success +# return 0 a body came back serverBody set, serverReason empty +# return 1 the call failed transport, or HTTP >= 400 +# return 2 answered 2xx, empty body for PHP this means a fatal error, but a +# few endpoints answer empty on purpose, +# so the caller decides which it is # -# It sets variables rather than echoing them because a caller needs BOTH the -# body and the description, and `x=$(postToServer ...)` runs in a subshell where -# any variable it set is discarded. Call it directly and read the three. +# serverBody the response body, empty if there was none +# serverStatus the HTTP status, 000 when curl never connected +# serverReason a human-readable description, empty only on return 0 +# +# GET when $2 is omitted or empty, POST otherwise -- the two are the same +# exchange and splitting them would duplicate all of the parsing below. # -# -w appends the status on its own line so an empty body stays distinguishable -# from a failed request; the body is everything before that last newline, so -# multi-line replies survive intact. +# It sets variables rather than echoing them because a caller needs BOTH the +# body and the description, and `x=$(callServer ...)` runs in a subshell where +# every variable it set is discarded. Call it directly and read the three. # -# $1 is the URL to post to -# $2 is the already-encoded post data -postToServer() { +# $1 is the URL +# $2 is the already-encoded post data, or empty for a GET +callServer() { local url="$1" - local data="$2" + local data="${2-}" local raw="" local rc=0 [[ -z $url ]] && handleError "No url passed (${FUNCNAME[0]})\n Args Passed: $*" serverBody="" serverStatus="000" serverReason="" - raw=$(curl -Lks -w $'\n%{http_code}' --data "$data" "$url" 2>/dev/null) + if [[ -n $data ]]; then + raw=$(curl -Lks -w $'\n%{http_code}' --data "$data" "$url" 2>/dev/null) + else + raw=$(curl -Lks -w $'\n%{http_code}' "$url" 2>/dev/null) + fi rc=$? if [[ $rc -ne 0 ]]; then serverReason="could not reach $url (curl exit $rc)" @@ -1706,13 +1715,23 @@ postToServer() { fi serverStatus="${raw##*$'\n'}" serverBody="${raw%$'\n'*}" + # Trailing newlines are stripped to match what every caller had before, when + # the body came straight out of `res=$(curl ...)` -- command substitution + # strips them, and the callers compare the result against a bare sentinel + # ("##", "##@GO", "#!ok"). Without this a server that ends its reply with a + # newline yields "##\n", every == "##" test fails, and a run that actually + # SUCCEEDED retries until it gives up. Only the -w status line is guaranteed + # to be newline-separated; anything before it belongs to the body. + while [[ ${serverBody} == *$'\n' ]]; do + serverBody="${serverBody%$'\n'}" + done if [[ $serverStatus -ge 400 || $serverStatus -eq 0 ]]; then serverReason="HTTP $serverStatus from $url" return 1 fi if [[ -z ${serverBody//[[:space:]]/} ]]; then serverReason="HTTP $serverStatus from $url with an EMPTY body -- the server accepted the request and answered with nothing. That is what a PHP fatal error looks like from here; the reason is in the web server's PHP error log, not on this screen." - return 1 + return 2 fi return 0 } @@ -3618,7 +3637,19 @@ restoreLVMPartition() { # The sender must emit this partition's LV files in sidecar order # (docs/adr/0007); against an older server the receivers would join # the wrong file's session, so refuse before the target is touched. - local servercaps=$(curl -Lks "${web}service/getversion.php?caps=1" 2>/dev/null) + # The CALL is checked before its answer is interpreted. This used to + # read the body straight out of $( ), so a server that could not be + # reached produced an empty $servercaps, which does not contain "mclvm", + # and the operator was told their server was too old to multicast LVM -- + # a claim about the server's VERSION drawn from what may have been a + # dead network. Same shape as fogproject GH-1266. The refusal itself is + # unchanged: without a positive mclvm we still stop before touching the + # target, because joining the wrong file's session is worse. + local servercaps="" + if ! callServer "${web}service/getversion.php?caps=1"; then + handleError "Cannot confirm the FOG server supports multicast deploy of LVM images: ${serverReason} (${FUNCNAME[0]})\n Args Passed: $*" + fi + servercaps="$serverBody" [[ $servercaps != *mclvm* ]] && handleError "The FOG server does not support multicast deploy of LVM images; update the server or deploy unicast (${FUNCNAME[0]})\n Args Passed: $*" fi local part_number=0 diff --git a/tests/README.md b/tests/README.md index 8d9021f2..15c11c6e 100644 --- a/tests/README.md +++ b/tests/README.md @@ -76,19 +76,27 @@ tests/checks/error-report.sh # the failure report handleError() sends to # reach its reboot notice, and no $web means no # attempt at all tests/checks/server-post-reporting.sh - # postToServer() and the two completion scripts - # (fogproject#1380): a connect failure, an HTTP - # >= 400, a 2xx with an EMPTY body and a real - # answer are told apart rather than all printing - # as one blank "Error returned:", an empty 200 - # points at the PHP error log because a PHP fatal - # is not catchable server-side, a multi-line body - # survives the -w status split, and -- the case - # that matters -- the caller can still read - # serverReason after the call, since $(postToServer - # ...) runs in a subshell that discards it. Both - # call sites are anchored whole-line for the same - # reason + # callServer() and every script that talks to the + # FOG server (fogproject#1380): a connect failure, + # an HTTP >= 400, a 2xx with an EMPTY body and a + # real answer are told apart rather than all + # printing as one blank "Error returned:", and the + # three get distinct return codes so a caller can + # tell a dead server from an endpoint that answers + # nothing on purpose. An empty 200 points at the + # PHP error log because a PHP fatal is not + # catchable server-side; a multi-line body survives + # the -w status split; trailing newlines are + # stripped, because every caller compares the reply + # against a bare sentinel ("##") and used to get + # that stripping free from $(curl ...). GET when no + # data is passed, POST when there is. And -- the + # case that matters -- the caller can still read + # serverReason after the call, since $(callServer + # ...) runs in a subshell that discards it, so no + # call site anywhere in the overlay may wrap it, + # and raw curl may read a reply only in the + # exceptions named in the check tests/checks/wipe.sh # wipeDisk() issues the right erase primitive per # device class (NVMe/SSD/HDD) and mode # (fast/normal/full), never issues an `nvme format` diff --git a/tests/checks/lvm.sh b/tests/checks/lvm.sh index 0c26265c..a66e5cc0 100755 --- a/tests/checks/lvm.sh +++ b/tests/checks/lvm.sh @@ -166,10 +166,20 @@ EOF # curl answers the multicast capability probe with the fake server's # response (a version string for an old server, a token list for a new one). +# +# The probe goes through callServer(), which asks for -w '\n%{http_code}' and +# reads the status off the LAST line, so the stub has to emit one. Without it +# the whole reply is taken as the status, compares as 0, and every capable-server +# case fails as an unreachable server -- which is what happened when the probe +# was converted. FAKE_HTTPCODE lets a case drive the refusal path deliberately. cat > "$STUBBIN/curl" <<'EOF' #!/bin/bash -echo "curl $*" >> "$CALLS" +# Newlines are folded out of the argv before logging: -w carries a literal +# newline, which would split the record across two lines and leave the URL on +# a line that no longer starts with "curl", so assert_call could never see it. +echo "curl ${*//$'\n'/ }" >> "$CALLS" printf '%s' "$FAKE_SERVERCAPS" +printf '\n%s' "${FAKE_HTTPCODE:-200}" exit 0 EOF @@ -309,7 +319,7 @@ run() { export FAKE_VG FAKE_PVUUID FAKE_PVSIZE FAKE_PVCOUNT FAKE_VGUUID FAKE_EXTENT export FAKE_LAYOUTS FAKE_LVS FAKE_SWAPUUID export FAKE_PESTART FAKE_PARTSIZE FAKE_FREE FAKE_EXTMIN FAKE_BLOCKSIZE - export FAKE_SERVERCAPS + export FAKE_SERVERCAPS FAKE_HTTPCODE . "$SANDBOX/funcs.sh" handleError() { echo "ABORT: $*"; exit 1; } imgFormat=5 @@ -591,8 +601,29 @@ restored=$(grep '^partclone.restore' "$CALLS" | sed -n 's#.*fakevg/\([a-z_0-9]*\ [[ $restored == "root home " ]] \ && ok "LVs restored in sidecar line order" \ || ko "restore order wrong: '$restored' (want 'root home ')" + assert_call "swap still regenerated locally, not multicast" "mkswap" "-U SWAPUUID-test" +# 30. Multicast deploy where the caps probe never reaches the server. The +# refusal is unchanged -- nothing may be touched without a positive mclvm -- +# but the operator must be told the CALL failed, not that their server is too +# old. The old code read the body out of $( ) with no status, so an unreachable +# server produced an empty $servercaps, which does not contain "mclvm", and the +# message blamed the server's version for what was a dead network. Same shape +# as fogproject GH-1266. +new_case 30 +lvm_image2 +FAKE_SERVERCAPS="mclvm" +FAKE_HTTPCODE=500 +run 'restoreLVMPartition /dev/sdb3 1 "$IMGDIR" yes' +FAKE_HTTPCODE="" +assert_out "unreachable server still refuses" "ABORT:" +assert_out "the refusal names the failed call" "Cannot confirm" +assert_out "and gives the HTTP status" "HTTP 500" +assert_not_out "it does not blame the server version" "does not support multicast deploy" +assert_no_call "nothing is touched" "pvcreate" +assert_no_call "no receiver is opened" "udp-receiver" + # 10. A sidecar from a newer FOS (unknown format version) refuses. new_case 10 lvm_image diff --git a/tests/checks/server-post-reporting.sh b/tests/checks/server-post-reporting.sh index 2c35b3c8..75a39fd9 100755 --- a/tests/checks/server-post-reporting.sh +++ b/tests/checks/server-post-reporting.sh @@ -1,6 +1,6 @@ #!/bin/bash # -# Assertion harness for postToServer() and the two completion scripts that use +# Assertion harness for callServer() and the two completion scripts that use # it. # # tests/checks/server-post-reporting.sh # run all cases, non-zero on failure @@ -20,7 +20,7 @@ # # fog.nonimgcomplete was worse: it printed no error text at all, ever. # -# postToServer() tells four outcomes apart that used to look identical: +# callServer() tells four outcomes apart that used to look identical: # # curl could not connect - transport, nothing reached the server # HTTP >= 400 - it answered, and refused @@ -30,9 +30,9 @@ # HTTP 2xx with a body - a real message # # The trap this harness exists to hold shut is the one that was WRITTEN and -# caught here during development: postToServer sets variables rather than +# caught here during development: callServer sets variables rather than # echoing them, because a caller needs both the body and the description, and -# `x=$(postToServer ...)` runs in a SUBSHELL where every variable it set is +# `x=$(callServer ...)` runs in a SUBSHELL where every variable it set is # discarded. That version looked correct, passed a read-through, and would have # reported an empty reason for every failure -- reintroducing the exact bug it # was written to fix. Case 6 drives it, and cases 7-8 pin both call sites. @@ -57,7 +57,7 @@ sed -e "s#^\. /usr/share/fog/lib/partition-funcs\.sh#. $SANDBOX/partition-funcs. STUBBIN="$SANDBOX/bin" mkdir -p "$STUBBIN" -# curl double. Emulates exactly the one thing postToServer depends on: with +# curl double. Emulates exactly the one thing callServer depends on: with # -w '\n%{http_code}' the status arrives on its own trailing line after the # body. FAKE_BODY may be empty or multi-line; FAKE_CURL_RC drives the transport # arm, in which case nothing is written at all, as real curl does on a connect @@ -84,7 +84,7 @@ note() { fi } -# Drives postToServer in THIS shell -- not a subshell -- and prints the three +# Drives callServer in THIS shell -- not a subshell -- and prints the three # variables plus the return code on one line each, so a case can assert on what # the caller would actually see. drive() { @@ -93,12 +93,16 @@ drive() { export PATH="$STUBBIN:$PATH" SANDBOX="$SANDBOX" export FAKE_CURL_RC FAKE_CODE FAKE_BODY . "$SANDBOX/funcs.sh" >/dev/null 2>&1 - postToServer "http://fog.example/service/Post_Stage2.php" "mac=00:11:22:33:44:55" + callServer "http://fog.example/service/Post_Stage2.php" "mac=00:11:22:33:44:55" rc=$? printf 'RC=%s\n' "$rc" printf 'STATUS=%s\n' "$serverStatus" printf 'REASON=%s\n' "$serverReason" printf 'BODY=%s\n' "$serverBody" + # %q renders a trailing newline as $'...\n' instead of swallowing it. + # BODY= above is line-based and `head -1` cannot see one at all, which + # is how the first cut of case 5b passed with the fix removed. + printf 'BODYQ=%q\n' "$serverBody" ) } @@ -124,7 +128,11 @@ note "2. an HTTP >= 400 answer is reported as a status, not as its body" "$err" # --- 3. the GH-1380 case: 200 with an empty body -------------------------- out=$(FAKE_CURL_RC=0 FAKE_CODE=200 FAKE_BODY="" drive) err="" -[[ $(field RC "$out") == 1 ]] || err="rc=$(field RC "$out")" +# 2, not 1: "it answered and gave me nothing" is a different fact from "the +# call never got there", and fog.man.reg's location/OU prompts branch on it -- +# an endpoint that legitimately has nothing to say must not read as a dead +# server. +[[ $(field RC "$out") == 2 ]] || err="rc=$(field RC "$out")" # Asserted on the REASON field, not on the whole capture: matching anywhere in # the output passes when the description is ECHOED rather than assigned, which # is the very shape that discards it in a subshell. @@ -148,15 +156,36 @@ err="" grep -q '^BODY=line one$' <<<"$out" || err="first line lost" note "5. only the LAST newline is the status separator" "$err" +# --- 5b. a trailing newline is stripped, as command substitution did ------ +# The regression this catches shipped once. Callers compare serverBody against a +# bare sentinel, and they used to get it from `res=$(curl ...)`, which strips +# trailing newlines. Keeping one turns a SUCCESSFUL run into eleven retries and +# a failure, because [[ $res == "##" ]] never matches "##\n". +out=$(FAKE_CURL_RC=0 FAKE_CODE=200 FAKE_BODY=$'##\n' drive) +err="" +q=$(field BODYQ "$out") +# Asserted on the %q form: "##" quoted is `\#\#` or `'##'`, never $'##\n'. +[[ $q == *'\n'* ]] && err="body still carries a newline: $q" +[[ $(field RC "$out") == 0 ]] || err="$err rc=$(field RC "$out")" +note "5b. a trailing newline is stripped, so == \"##\" still matches" "$err" + +# Repeated trailing newlines all go; an interior one stays. +out=$(FAKE_CURL_RC=0 FAKE_CODE=200 FAKE_BODY=$'one\ntwo\n\n' drive) +err="" +q=$(field BODYQ "$out") +[[ $q == *'two'*'\n'* ]] && err="trailing newline survived: $q" +[[ $q == *'one'*'\n'*'two'* ]] || err="$err interior newline lost: $q" +note "5b. repeated trailing newlines go, interior ones stay" "$err" + # --- 6. the variables reach the caller ------------------------------------ -# The bug this whole file exists to hold shut. If postToServer is ever changed +# The bug this whole file exists to hold shut. If callServer is ever changed # back to echoing its description, or a caller wraps it in $( ), serverReason is # empty in the caller and the client prints a blank error again. out=$( set +u export PATH="$STUBBIN:$PATH" SANDBOX="$SANDBOX" FAKE_CODE=200 FAKE_BODY="" . "$SANDBOX/funcs.sh" >/dev/null 2>&1 - postToServer "http://fog.example/x.php" "a=b" + callServer "http://fog.example/x.php" "a=b" # Deliberately read AFTER the call returns, in the calling scope. printf 'SEEN=%s\n' "${serverReason:-}" ) @@ -171,15 +200,85 @@ note "6. the caller can read serverReason after the call returns" "$err" for f in fog.imgcomplete fog.nonimgcomplete; do err="" [[ -f $REPO_BIN/$f ]] || { note "7. $f exists" "missing"; continue; } - if grep -Eq '^[[:space:]]*[A-Za-z_]+=\$\(postToServer' "$REPO_BIN/$f"; then - err="calls postToServer inside \$( ), which discards its variables" - elif ! grep -Eq '^[[:space:]]*postToServer ' "$REPO_BIN/$f"; then - err="does not call postToServer directly" + if grep -Eq '^[[:space:]]*[A-Za-z_]+=\$\(callServer' "$REPO_BIN/$f"; then + err="calls callServer inside \$( ), which discards its variables" + elif ! grep -Eq '^[[:space:]]*callServer ' "$REPO_BIN/$f"; then + err="does not call callServer directly" elif ! grep -q 'serverReason' "$REPO_BIN/$f"; then err="never prints serverReason, so a failure still says nothing" fi - note "7. $f calls postToServer directly and prints its reason" "$err" + note "7. $f calls callServer directly and prints its reason" "$err" +done + +# --- 9. GET when no data is given, POST when there is --------------------- +# One helper serves both because it is the same exchange; the only difference +# that matters is whether --data is on the argv. A GET site that quietly grew a +# --data would post an empty body to an endpoint expecting a query string. +( + set +u + export PATH="$STUBBIN:$PATH" SANDBOX="$SANDBOX" FAKE_CODE=200 FAKE_BODY="x" + . "$SANDBOX/funcs.sh" >/dev/null 2>&1 + callServer "http://fog.example/get.php" +) >/dev/null 2>&1 +err="" +grep -Fxq -- "--data" "$SANDBOX/curl.argv" && err="--data was on the argv" +note "9. a call with no data is a GET" "$err" + +( + set +u + export PATH="$STUBBIN:$PATH" SANDBOX="$SANDBOX" FAKE_CODE=200 FAKE_BODY="x" + . "$SANDBOX/funcs.sh" >/dev/null 2>&1 + callServer "http://fog.example/post.php" "a=b" +) >/dev/null 2>&1 +err="" +grep -Fxq -- "--data" "$SANDBOX/curl.argv" || err="no --data" +grep -Fxq -- "a=b" "$SANDBOX/curl.argv" || err="$err; data not passed" +note "9. a call with data is a POST carrying it" "$err" + +# --- 10. three distinct return codes -------------------------------------- +# 1 (the call failed) and 2 (answered, empty body) have to be separable, or a +# caller cannot tell a dead server from an endpoint that legitimately answers +# nothing -- which is exactly what fog.man.reg's location/OU prompts need. +out=$(FAKE_CURL_RC=7 drive); rc_fail=$(field RC "$out") +out=$(FAKE_CURL_RC=0 FAKE_CODE=500 FAKE_BODY="x" drive); rc_http=$(field RC "$out") +out=$(FAKE_CURL_RC=0 FAKE_CODE=200 FAKE_BODY="" drive); rc_empty=$(field RC "$out") +out=$(FAKE_CURL_RC=0 FAKE_CODE=200 FAKE_BODY="##" drive); rc_ok=$(field RC "$out") +err="" +[[ $rc_ok == 0 ]] || err="ok=$rc_ok" +[[ $rc_fail == 1 ]] || err="$err transport=$rc_fail" +[[ $rc_http == 1 ]] || err="$err http500=$rc_http" +[[ $rc_empty == 2 ]] || err="$err empty=$rc_empty" +note "10. 0=body, 1=call failed, 2=answered but empty" "$err" + +# --- 11. every converted call site, whole-line anchored ------------------- +# A name grep passes when the call has been wrapped in $( ), which discards +# every variable it set -- the failure this whole file exists to prevent. So no +# site anywhere in the overlay may assign from one. +OVERLAY="$REPO_BIN/.." +sites=$(grep -rl 'callServer ' "$OVERLAY" 2>/dev/null) +err="" +[[ -n $sites ]] || err="no call sites found at all" +for f in $sites; do + # Comment lines are stripped first: funcs.sh documents this very trap in + # prose above the definition, and a naive grep flags its own warning. + sed 's/#.*//' "$f" | grep -Eq '\$\(callServer|`callServer' && err="$err $(basename "$f")" done +note "11. no call site wraps callServer in \$( ) ($(wc -w <<<"$sites") files)" "$err" + +# Nothing outside the documented exceptions may go back to a bare curl that +# reads a reply. The exceptions are listed by name so adding one is a decision +# someone makes here, not a thing that drifts back in unnoticed. Each is +# justified in a comment at its own call site. +err="" +while read -r hit; do + [[ -n $hit ]] || continue + b=$(basename "${hit%%:*}") + case "$b" in + funcs.sh|fog.statusreporter|fog.av|fog|secureboot-funcs.sh|S40network) continue ;; + esac + err="$err $b" +done < <(grep -rn '=\$(curl \|=`curl ' "$OVERLAY" 2>/dev/null) +note "11. raw curl reads a reply only in the documented exceptions" "$err" echo echo "passed: $PASS failed: $FAIL"