feat(hack): improve check-readiness.sh — wait/core/parallel modes and broader coverage - #2740

⚠️ Potential issue | 🔴 Critical | ⚡ Quick win

Don’t treat kubectl failures as “ready.”

api-resources, per-kind get, and parallel wait errors are all swallowed here, so an unreachable apiserver or forbidden LIST can degrade into empty row files and a final All resources are ready. That’s a false green on the script’s main path.

Also applies to: 275-275, 367-367

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@hack/check-readiness.sh` around lines 193 - 209, The script currently
swallows kubectl failures (via constructs like || true and ignoring empty
caches) which causes unreachable API server or forbidden LIST to be treated as
"ready"; update ensure_api_resources_cached to return a non-zero failure when
the kubectl api-resources call fails (don't use "|| true"), propagate that error
to callers, and change kind_exists so it distinguishes between an empty result
and a real kubectl failure (return non-zero on failure instead of assuming the
kind exists); likewise, in the per-kind get and the parallel wait logic (the
other places noted) stop discarding kubectl exit codes, check and propagate
those errors up so the script exits non-zero on API errors rather than printing
"All resources are ready."

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

#!/bin/bash
set -euo pipefail

rows="$(mktemp)"
printf 'default\037helmrelease-a\037False\037InstallFailed\037first line\nsecond line\037false\n' > "$rows"

while IFS=$'\x1f' read -r ns name status reason message suspended; do
  printf 'ns=%q name=%q status=%q reason=%q message=%q suspended=%q\n' \
    "$ns" "$name" "$status" "$reason" "$message" "$suspended"
done < "$rows"

Fix row framing for multiline message values in hack/check-readiness.sh
fetch_rows currently terminates each row with a newline ({"\n"}) while .status.conditions[..].message is emitted verbatim; a multiline message will be split into multiple records and corrupt the parsed columns (e.g., first line becomes one row and second line shifts into subsequent fields).

-# Returns one row per object, fields separated by ASCII Unit Separator (0x1f):
-#   namespace<US>name<US>status<US>reason<US>message<US>suspended
+# Returns one row per object, fields separated by ASCII Unit Separator (0x1f)
+# and records separated by ASCII Record Separator (0x1e):
+#   namespace<US>name<US>status<US>reason<US>message<US>suspended<RS>
@@
-    jsonpath='{range .items[*]}{.metadata.namespace}{"\x1f"}{.metadata.name}{"\x1f"}{.status.phase}{"\x1f"}{""}{"\x1f"}{""}{"\x1f"}{"false"}{"\n"}{end}'
+    jsonpath='{range .items[*]}{.metadata.namespace}{"\x1f"}{.metadata.name}{"\x1f"}{.status.phase}{"\x1f"}{""}{"\x1f"}{""}{"\x1f"}{"false"}{"\x1e"}{end}'
@@
-    jsonpath+="${suspend_field}"'{"\n"}'
+    jsonpath+="${suspend_field}"'{"\x1e"}'
@@
-  while IFS=$'\x1f' read -r ns name status reason message suspended; do
+  while IFS=$'\x1f' read -r -d $'\x1e' ns name status reason message suspended; do

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@hack/check-readiness.sh` around lines 242 - 275, The jsonpath currently
terminates rows with a literal newline {"\n"}, which breaks rows when
.status.conditions[].message contains newlines; change the record terminator to
a non-printing separator (e.g., ASCII Record Separator \x1e) in the jsonpath
construction inside fetch_rows (replace {"\n"} with {"\x1e"}), then after the
kubectl call convert that separator back to newlines (pipe the KUBECTL get ...
output through tr $'\x1e' '\n' or equivalent) so multiline message fields stay
contained in a single logical row; update the jsonpath strings and the
"${KUBECTL[@]}" get "$kind" ... invocation accordingly.

Appending `|| true` to the `kubectl get` command in `fetch_rows` silently ignores any actual errors (such as authentication failures, network timeouts, or API server unavailability). If `kubectl` fails, it outputs nothing, which causes `process_rows` to see an empty file and assume all resources of that kind are ready. This can lead to dangerous false positives where the script reports "All resources are ready" when the cluster is actually unreachable or experiencing issues. Since `kind_exists` is already checked beforehand, any failure here is a genuine error and should be propagated.

Currently, `run_once` does not return an explicit exit status, meaning it always returns `0` (success) because the last evaluated statement (the `if` block when `found_issues` is non-zero) evaluates to `0` in Bash. This forces the `WAIT_MODE` loop to use a fragile `grep` on the stdout (`grep -q "All resources are ready."`). If any resource's status message or namespace happens to contain that string, the script will falsely report success and exit prematurely. Explicitly returning `0` or `1` from `run_once` allows us to check the exit status directly and safely.

Instead of grepping the stdout of `run_once` for a magic string (which is prone to false positives if any resource name, namespace, or error message contains `"All resources are ready."`), we should check the exit status of `run_once` directly. This is much more robust and idiomatic.

The `usage()` function relies on hardcoded line numbers (`sed -n '2,24p'`). This is fragile because any future changes to the script's header comments, adding/removing lines, or modifying the script's structure will silently break the help output. We can make this robust by using pattern matching to extract the help text dynamically.

In `WATCH` mode, the cursor is hidden using `tput civis`. If the script exits due to an error (e.g., a command fails and `set -e` triggers) or is terminated by another signal, the cursor will remain hidden in the user's terminal. Using an `EXIT` trap ensures that the cursor is always restored (`tput cnorm`) regardless of how the script exits.

If `tput` is installed but the terminal capability database is missing or `TERM` is invalid, `tput cup` or `tput ed` can fail and print error messages to stderr without falling back to `clear`. We should redirect stderr to `/dev/null` and fall back to `clear` if the `tput` commands fail.

Newline in condition messages breaks parsing & Silenced kubectl errors

1. Newline Issue: Kubernetes condition messages (especially for failing HelmReleases or Kustomizations) frequently contain newlines. Since fetch_rows uses jsonpath to output raw fields and process_rows reads line-by-line, any newline inside a message will break the row parsing. This leads to corrupted output (e.g., printing parts of the message as non-existent resource names with status Unknown).
2. Silenced Errors: Appending || true and redirecting stderr to /dev/null in fetch_rows causes any real kubectl errors (such as authentication failure, network timeout, or permission denied) to be silently ignored. This results in empty output files, which process_rows treats as "no issues", ultimately leading the script to falsely report "All resources are ready." and exit with 0.

Solution

Using jq (which is already a project dependency) to parse the output of kubectl get -o json is much more robust. It allows us to safely replace newlines in messages with spaces using gsub("\n"; " "), handle missing fields gracefully, and propagate real connection/auth errors correctly.

fetch_rows() {
  local kind="$1" scope="$2" cond_type="$3" supports_suspend="$4"
  local args=()
  build_kubectl_scope_args "$scope" args

  if [[ "$cond_type" == "_phase" ]]; then
    "${KUBECTL[@]}" get "$kind" "${args[@]}" -o json | jq -r '.items[]? | [
      (.metadata.namespace // ""),
      .metadata.name,
      (.status.phase // ""),
      "",
      "",
      "false"
    ] | join("\u001f")'
  else
    "${KUBECTL[@]}" get "$kind" "${args[@]}" -o json | jq -r --arg cond "$cond_type" --arg susp "$supports_suspend" '.items[]? | [
      (.metadata.namespace // ""),
      .metadata.name,
      ([.status.conditions[]? | select(.type == $cond)][0] | .status // ""),
      ([.status.conditions[]? | select(.type == $cond)][0] | .reason // ""),
      ([.status.conditions[]? | select(.type == $cond)][0] | .message // "" | gsub("\n"; " ")),
      (if $susp == "1" then .spec.suspend // false else false end | tostring)
    ] | join("\u001f")'
  fi
}

Stale API Resources Cache during Upgrades

__API_RESOURCES_CACHED is a global variable and is never reset to 0. During a watch or wait loop, if a new CRD is installed (which is common during upgrades), the script will never detect it and will continue to skip it. Resetting __API_RESOURCES_CACHED=0 at the start of run_once ensures the cache is refreshed per iteration.

tput failure under set -e crashes the script

Under set -e, if tput cup 0 0 or tput ed fails (which happens in environments with a dumb or missing TERM variable, such as some CI/CD runners or cron jobs), the script will immediately crash. Wrapping them in a safe && chain or appending || true / || clear prevents this.

Temporary directory leak on interrupt / failure

If the script is interrupted (e.g., via Ctrl+C in watch mode) or fails due to set -e while run_once is running, the temporary directory created by mktemp will be leaked in /tmp. Registering a global cleanup trap on EXIT and tracking the current temp directory ensures it is always cleaned up.

Reset CURRENT_TMPDIR after successful cleanup at the end of run_once.

In Kubernetes, status condition messages (especially for failing HelmReleases or Kustomizations) often contain newlines. Since the script currently uses \n as the record separator, any newline inside a condition message will prematurely terminate the read command, causing subsequent fields and rows to be misaligned or skipped entirely.

We can solve this robustly by using the ASCII Record Separator (\x1e) as the record delimiter instead of \n.

Use read -d $'\x1e' to read records delimited by the ASCII Record Separator (\x1e), ensuring that multiline status messages do not break the parsing loop.

The usage() function hardcodes the line numbers (2,24p) to extract the help comment block. This is fragile and will break if any comments or lines are added or removed at the top of the file. We can make this dynamic and robust by using awk to print all comment lines starting from line 2 until the first non-comment line.

The temporary directory tmpdir is created in run_once() but is only cleaned up at the very end of the function. If the script exits early (e.g., due to set -e on a command failure or user interruption), the temporary directory will be left behind in /tmp.

Using an EXIT trap with double quotes ensures that the temporary directory is always cleaned up when the shell/subshell exits, even if the local variable goes out of scope.

In watch mode, the terminal cursor is hidden using tput civis. The current trap only restores the cursor (tput cnorm) on INT and TERM signals. If the script exits due to an error (e.g., a command failure under set -e), the cursor will remain hidden, which degrades the user's terminal experience.

Using an EXIT trap guarantees that the cursor is always restored when the script exits for any reason.