run-with-macadam.sh (2)

284-292: Fragile regex extraction of SSH port from gvproxy.

Similar to the socket path extraction, the sed regex sed -n 's/.*-ssh-port \([0-9]*\).*/\1/p' is brittle. If the command-line format changes, this fails silently.

Verify the format of gvproxy's -ssh-port flag and consider extracting both socket and port using a more robust approach (e.g., structured parsing or environment variables).

---

277-278: Hard-coded 180-second sleep inflexible for varying infra.

The sleep 180 assumes all VMs take exactly 3 minutes to boot, which may not hold across different hardware or CI runners. For slower infra, the wait is insufficient; for faster infra, it wastes time. Consider polling with timeout instead.

echo "Waiting for VM to start (up to 10 minutes)..."
local timeout=600
local elapsed=0
while [[ $elapsed -lt $timeout ]]; do
    if macadam ssh crc-ng -- exit 0 2>/dev/null; then
        echo "VM is reachable."
        break
    fi
    sleep 5
    ((elapsed += 5))
done
if [[ $elapsed -ge $timeout ]]; then
    die "VM did not become reachable within ${timeout}s."
fi

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

run-with-macadam.sh (6)

17-22: cleanup() exits early on macadam failure, blocking file cleanup.

The macadam rm command at line 21 fails if macadam is unavailable or the VM doesn't exist. Since cleanup() runs under set -e, it exits with error status before reaching the rm -f statements, leaving temporary files behind and violating the trap contract.

Apply this diff:

  function cleanup() {
      echo "--- Cleaning up ---"
      rm -f user-data crc.img crc.qcow2 bundle.tar kubeconfig "${PRIV_KEY_PATH}" "${PUB_KEY_PATH}"
-     macadam rm crc-ng --force
+     macadam rm crc-ng --force 2>/dev/null || true
  }

---

149-164: macadam version not pinned; "latest" tag breaks reproducibility and complicates CI debugging.

Line 151 uses the mutable latest release tag, which allows different CI runs to download different versions. This prevents reproducible builds and complicates root-cause analysis when failures occur. Additionally, lines 155 and 159 invoke sudo without verifying passwordless access; if the CI environment requires a password, the script will hang or fail silently.

Pin the macadam version and verify passwordless sudo:

+ # Ensure passwordless sudo is available for macadam installation
+ if ! sudo -n true 2>/dev/null; then
+     die "This script requires passwordless sudo access to install macadam to /opt/macadam."
+ fi
+
- local version="latest"
+ # Pin to a specific stable version for reproducibility; update when macadam releases compatible changes
+ local version="v0.0.1"  # Replace with current stable version
  local url="https://github.com/crc-org/macadam/releases/download/${version}/macadam-linux-amd64"

Alternatively, install to a user-writable directory (e.g., $HOME/.local/bin) to avoid sudo altogether.

---

58-72: xtrace disabled but never restored, impairing script debugging.

Line 61 disables xtrace to protect secrets, but there is no corresponding restoration. If the script is invoked with bash -x for debugging, tracing remains disabled for all subsequent code, hindering troubleshooting.

Apply this diff to capture and restore xtrace state:

  function load_resources() {
      # Unset xtrace to prevent secrets from being exposed if the script is run with -x
+     local xtrace_was_set=0
+     if [[ "$-" == *x* ]]; then
+         xtrace_was_set=1
+     fi
      set +x
      echo "--- Loading resources ---"
      if [[ -z "${PULL_SECRET_PATH}" || ! -f "${PULL_SECRET_PATH}" ]]; then
          die "Path to pull secret file must be set via PULL_SECRET_PATH, and the file must exist."
      fi
      if [[ ! -f "${PUB_KEY_PATH}" ]]; then
          die "Public key file not found at ${PUB_KEY_PATH}. This should have been generated."
      fi
  
      PULL_SECRET=$(cat "${PULL_SECRET_PATH}")
      PUB_KEY=$(cat "${PUB_KEY_PATH}")
+     if [[ $xtrace_was_set -eq 1 ]]; then
+         set -x
+     fi
  }

---

180-182: macadam start failure does not halt execution, causing silent cascade failures.

When macadam start crc-ng fails (line 180), the script only echoes a message and continues. Downstream functions (DNS setup, port forwarding, kubeconfig retrieval) will execute against a non-running VM, causing confusing failures downstream instead of a clear error at the point of failure.

Replace with immediate termination:

- if ! macadam start crc-ng --log-level debug; then
-     echo "Machine didn't come up in time"
- fi
+ if ! macadam start crc-ng --log-level debug; then
+     die "Machine didn't come up in time"
+ fi

---

230-237: Regex for gvproxy socket path extraction is fundamentally broken.

The sed regex at line 234 ('s/.*-services unix:\/\/\([^ ]*\).*/\1/p') does not match actual gvproxy command-line output. Gvproxy uses --listen unix:///path (double dash, triple slashes), but the regex looks for -services unix:// (single dash, double slashes). This regex will silently fail to extract the socket path, causing the check at line 235 to die with a generic error message that masks the real problem.

Replace with correct extraction targeting the --listen flag:

- GVPROXY_SOCKET_PATH=$(echo "${gvproxy_cmd}" | sed -n 's/.*-services unix:\/\/\([^ ]*\).*/\1/p')
+ GVPROXY_SOCKET_PATH=$(echo "${gvproxy_cmd}" | sed -n 's/.*--listen unix:\/\/\/\([^ ]*\).*/\1/p')

Verify this against actual gvproxy process output (e.g., pgrep -af gvproxy) to confirm the format matches.

---

74-114: xtrace disabled but never restored; credentials written with world-readable permissions.

Line 79 disables xtrace to protect secrets in the cloud-init heredoc, but it is never restored, compounding the issue from load_resources(). Additionally, sensitive credentials at lines 100–108 (pull-secret, pass_kubeadmin, pass_developer) are written with permissions: '0644', making them world-readable. These should be restricted to 0600 (owner only).

Apply this diff:

  function gen_cloud_init() {
      echo "--- Generating cloud-init user-data ---"
      rm -f user-data
      # Unset xtrace to prevent secrets from being exposed if the script is run with -x
+     local xtrace_was_set=0
+     if [[ "$-" == *x* ]]; then
+         xtrace_was_set=1
+     fi
      set +x
      cat <<EOF >user-data
  #cloud-config
  runcmd:
    - systemctl enable --now kubelet
  write_files:
  - path: /home/core/.ssh/authorized_keys
    content: '$PUB_KEY'
    owner: core
    permissions: '0600'
  - path: /opt/crc/id_rsa.pub
    content: '$PUB_KEY'
    owner: root:root
    permissions: '0644'
  - path: /etc/sysconfig/crc-env
    content: |
      CRC_SELF_SUFFICIENT=1
      CRC_NETWORK_MODE_USER=1
    owner: root:root
    permissions: '0644'
  - path: /opt/crc/pull-secret
    content: |
      $PULL_SECRET
-   permissions: '0644'
+   permissions: '0600'
  - path: /opt/crc/pass_kubeadmin
    content: '$PASS_KUBEADMIN'
-   permissions: '0644'
+   permissions: '0600'
  - path: /opt/crc/pass_developer
    content: '$PASS_DEVELOPER'
-   permissions: '0644'
+   permissions: '0600'
  - path: /opt/crc/ocp-custom-domain.service.done
    permissions: '0644'
  EOF
      USER_DATA_PATH="$(pwd)/user-data"
      echo "cloud-init user-data file created."
+     if [[ $xtrace_was_set -eq 1 ]]; then
+         set -x
+     fi
  }

run-with-macadam.sh (2)

257-298: SSH port extraction relies on fragile regex pattern.

The regex at line 272 ('s/.*-ssh-port \([0-9]*\).*/\1/p') extracts the SSH port from gvproxy command-line output. While this pattern is simpler and likely more robust than the socket path extraction, it still assumes a fixed flag format and no embedded spaces. If gvproxy's command-line interface evolves, this extraction will silently fail. At a minimum, verify this regex against actual pgrep -af gvproxy output for your environment.

Consider validating the extracted port (ensure it's a valid port number between 1–65535) to catch extraction failures early:

if [[ -z "${ssh_port}" || ! "${ssh_port}" =~ ^[0-9]+$ ]] || (( ssh_port < 1 || ssh_port > 65535 )); then
    die "Invalid SSH port extracted from gvproxy: ${ssh_port}"
fi

---

1-16: Script setup and global variables are well-organized.

The use of set -euo pipefail enables strict error handling. Constants are properly marked readonly, and mutable globals are clearly documented. The default bundle path includes an architecture-specific reference (arm64); consider making this configurable or architecture-agnostic for broader CI portability.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Learnt from: kpouget
Repo: crc-org/snc PR: 1168
File: systemd/dnsmasq.sh.template:3-12
Timestamp: 2025-10-02T13:13:28.229Z
Learning: The file `/etc/sysconfig/crc-env` in the crc-org/snc repository contains configuration variables and file paths (e.g., `CRC_PASS_KUBEADMIN_PATH`, `CRC_PUB_KEY_PATH`, `CRC_NETWORK_MODE_USER`, `APPS_DOMAIN`), not the actual secrets. The secrets themselves are stored in separate files referenced by these paths. Sourcing `crc-env` with xtrace enabled does not leak sensitive information.

Learnt from: kpouget
Repo: crc-org/snc PR: 1168
File: systemd/crc-self-sufficient-env.sh:10-17
Timestamp: 2025-10-07T07:03:06.884Z
Learning: In the crc-org/snc repository, the CRC_CLOUD environment variable has been removed from the codebase. The self-sufficient environment check in systemd/crc-self-sufficient-env.sh now only validates CRC_SELF_SUFFICIENT=1, not CRC_CLOUD.

run-with-macadam.sh (5)

59-72: Restore xtrace setting after loading secrets.

The function disables xtrace at line 61 but never restores it, so tracing remains disabled if the script was invoked with bash -x. This breaks debugging for the remainder of the script execution.

Apply this diff to capture and restore the original trace state:

 function load_resources() {
     # Unset xtrace to prevent secrets from being exposed if the script is run with -x
+    local xtrace_state
+    xtrace_state=$([[ $- == *x* ]] && echo "set -x" || echo "set +x")
     set +x
     echo "--- Loading resources ---"
     if [[ -z "${PULL_SECRET_PATH}" || ! -f "${PULL_SECRET_PATH}" ]]; then
         die "Path to pull secret file must be set via PULL_SECRET_PATH, and the file must exist."
     fi
     if [[ ! -f "${PUB_KEY_PATH}" ]]; then
         die "Public key file not found at ${PUB_KEY_PATH}. This should have been generated."
     fi
 
     PULL_SECRET=$(cat "${PULL_SECRET_PATH}")
     PUB_KEY=$(cat "${PUB_KEY_PATH}")
+    eval "$xtrace_state"
 }

---

75-117: Restore xtrace and fix credentials file permissions.

Two issues in this function:

1. Trace restoration (line 79): Like load_resources(), xtrace is disabled but never restored, breaking debugging.
2. World-readable secrets (lines 100-108): Files containing credentials (pull-secret, pass_kubeadmin, pass_developer) are written with 0644 permissions, making them readable by any user on the system. These should be 0600.

Apply this diff to fix both issues:

 function gen_cloud_init() {
     echo "--- Generating cloud-init user-data ---"
     rm -f user-data
     # Unset xtrace to prevent secrets from being exposed if the script is run with -x
+    local xtrace_state
+    xtrace_state=$([[ $- == *x* ]] && echo "set -x" || echo "set +x")
     set +x
     cat <<EOF >user-data
 #cloud-config
 runcmd:
   - systemctl enable --now kubelet
 write_files:
 - path: /home/core/.ssh/authorized_keys
   content: '$PUB_KEY'
   owner: core
   permissions: '0600'
 - path: /opt/crc/id_rsa.pub
   content: '$PUB_KEY'
   owner: root:root
   permissions: '0644'
 - path: /etc/sysconfig/crc-env
   content: |
     CRC_SELF_SUFFICIENT=1
     CRC_NETWORK_MODE_USER=1
   owner: root:root
   permissions: '0644'
 - path: /opt/crc/pull-secret
   content: |
     $PULL_SECRET
-  permissions: '0644'
+  permissions: '0600'
 - path: /opt/crc/pass_kubeadmin
   content: '$PASS_KUBEADMIN'
-  permissions: '0644'
+  permissions: '0600'
 - path: /opt/crc/pass_developer
   content: '$PASS_DEVELOPER'
-  permissions: '0644'
+  permissions: '0600'
 - path: /opt/crc/ocp-custom-domain.service.done
   permissions: '0644'
 EOF
     USER_DATA_PATH="$(pwd)/user-data"
     if ! cloud-init schema -c "${USER_DATA_PATH}" --annotate; then
         die "Invalid cloud-config generated."
     fi
     echo "cloud-init user-data file created."
+    eval "$xtrace_state"
 }

---

139-167: Pin macadam version and verify passwordless sudo access.

Two issues in this function:

1. Mutable version tag (lines 154–155): Using "latest" is not reproducible and can introduce breaking changes between CI runs. Pin to a specific stable release.
2. Unverified sudo privilege (lines 158–162): The script calls sudo mkdir -p /opt/macadam and sudo mv without checking if passwordless sudo is available. In CI environments where sudo requires a password, this will hang or fail silently.

Apply this diff to pin the version and verify sudo privileges:

     echo "macadam not found, downloading..."
 
-    local version="latest"
+    local version="v0.1.0"  # Pin to a stable version; update when new releases are validated
     local url="https://github.com/crc-org/macadam/releases/download/${version}/macadam-linux-amd64"
 
     echo "Downloading from ${url}"
+    if ! sudo -n true 2>/dev/null; then
+        die "This script requires passwordless sudo access. Please configure sudo NOPASSWD for this user."
+    fi
     sudo mkdir -p /opt/macadam
     if ! curl -fL -o /tmp/macadam "${url}"; then
         die "Failed to download macadam."
     fi
     sudo mv /tmp/macadam /opt/macadam/macadam
     sudo chmod +x /opt/macadam/macadam
 
     export PATH="/opt/macadam:$PATH"
     echo "macadam downloaded to /opt/macadam and added to PATH."

Documentation note: After determining the stable macadam version, update the version variable and document it in the PR description or a configuration file.

---

198-214: Exit on macadam VM startup failure.

When macadam start fails (line 211), the script logs a message but continues execution, causing downstream operations to silently fail. This should halt the script immediately.

Apply this diff to exit on failure:

 function start_macadam_vm() {
     echo "--- Creating VM ---"
     macadam init \
         "${DISK_IMAGE_PATH}" \
         --disk-size 31 \
         --memory 11264 \
         --name crc-ng \
         --username core \
         --ssh-identity-path "${PRIV_KEY_PATH}" \
         --cpus 6 \
         --cloud-init "${USER_DATA_PATH}" \
         --log-level debug
 
     if ! macadam start crc-ng --log-level debug; then
-        echo "Machine didn't come up in time"
+        die "Machine didn't come up in time"
     fi
 }

---

274-325: Fix gvproxy socket path extraction regex.

The regex at line 304 is fundamentally broken and will not match actual gvproxy command-line output:

# Current (incorrect):
sed -n 's/.*-services unix:\/\/\([^ ]*\).*/\1/p'

# Expected gvproxy format:
gvproxy --services --listen unix:///tmp/network.sock

Issues:

• Looks for -services (single dash) but the flag is --services (double dash)
• Looks for unix:// (two slashes) but the actual format uses unix:/// (three slashes for UNIX socket paths)
• The regex will fail silently and extract nothing, causing the script to die with a confusing error

Apply this diff to extract the socket path from the correct --listen flag:

     local gvproxy_cmd
     if ! gvproxy_cmd=$(pgrep -af gvproxy); then
         die "The gvproxy process is not running."
     fi
 
-    GVPROXY_SOCKET_PATH=$(echo "${gvproxy_cmd}" | sed -n 's/.*-services unix:\/\/\([^ ]*\).*/\1/p')
+    # Extract the socket path from the --listen flag (format: --listen unix:///path/to/socket)
+    GVPROXY_SOCKET_PATH=$(echo "${gvproxy_cmd}" | sed -n 's/.*--listen unix:\/\/\/\([^ ]*\).*/\1/p')
     if [[ -z "${GVPROXY_SOCKET_PATH}" ]]; then
         die "Could not find the gvproxy socket path."
     fi

For additional robustness, consider validating that the extracted path exists and is a UNIX socket:

if [[ ! -S "${GVPROXY_SOCKET_PATH}" ]]; then
    die "Extracted gvproxy socket path does not exist or is not a UNIX socket: ${GVPROXY_SOCKET_PATH}"
fi

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Learnt from: kpouget
Repo: crc-org/snc PR: 1168
File: systemd/dnsmasq.sh.template:3-12
Timestamp: 2025-10-02T13:13:28.229Z
Learning: The file `/etc/sysconfig/crc-env` in the crc-org/snc repository contains configuration variables and file paths (e.g., `CRC_PASS_KUBEADMIN_PATH`, `CRC_PUB_KEY_PATH`, `CRC_NETWORK_MODE_USER`, `APPS_DOMAIN`), not the actual secrets. The secrets themselves are stored in separate files referenced by these paths. Sourcing `crc-env` with xtrace enabled does not leak sensitive information.

Learnt from: kpouget
Repo: crc-org/snc PR: 1168
File: systemd/crc-self-sufficient-env.sh:10-17
Timestamp: 2025-10-07T07:03:06.884Z
Learning: In the crc-org/snc repository, the CRC_CLOUD environment variable has been removed from the codebase. The self-sufficient environment check in systemd/crc-self-sufficient-env.sh now only validates CRC_SELF_SUFFICIENT=1, not CRC_CLOUD.

run-with-macadam.py (4)

174-174: Unused config parameter.

config is never referenced in this function. Remove it or use _config to signal intent.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@run-with-macadam.py` at line 174, The function generate_ssh_keypair currently
declares an unused parameter config; remove this parameter from the function
signature (and any callers) or rename it to _config to signal intentional
non-use. Update the function definition generate_ssh_keypair(...) and any places
that call it (search for generate_ssh_keypair) so signatures and calls match the
chosen change.

---

481-484: Unconditional 3-minute sleep before SSH readiness check.

The wait_for_ssh() call on line 489 already polls for SSH. The fixed 180-second sleep adds unnecessary latency when the VM boots faster. Consider reducing or removing it and relying on wait_for_ssh() with a backoff.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@run-with-macadam.py` around lines 481 - 484, The unconditional 180s sleep in
get_kubeconfig is causing unnecessary latency; instead remove (or drastically
reduce) the fixed time.sleep(180) and rely on the existing wait_for_ssh()
polling/backoff logic to detect SSH readiness (or, if needed, replace the long
sleep with a short initial sleep like 5–10s before calling wait_for_ssh()).
Update get_kubeconfig to call wait_for_ssh() directly (or after the short sleep)
so the function uses the exponential/backoff retry behavior rather than a hard
3-minute pause.

---

386-397: get_gvproxy_socket_path() and get_ssh_port() share identical gvproxy process lookup.

Both functions call pgrep -af gvproxy and parse stdout. Extract the gvproxy command line retrieval into a shared helper to avoid duplication and reduce the chance of one diverging from the other.

Suggested refactor

+def _get_gvproxy_cmdline() -> str:
+    try:
+        result = run_command(["pgrep", "-af", "gvproxy"], capture_output=True)
+    except MacadamError as e:
+        raise MacadamError("The gvproxy process is not running.") from e
+    return result.stdout.strip()
+
 def get_gvproxy_socket_path() -> str:
-    try:
-        result = run_command(["pgrep", "-af", "gvproxy"], capture_output=True)
-    except MacadamError as e:
-        raise MacadamError("The gvproxy process is not running.") from e
-
-    gvproxy_cmd = result.stdout.strip()
+    gvproxy_cmd = _get_gvproxy_cmdline()
     match = re.search(r'-services unix://([^ ]+)', gvproxy_cmd)
     if not match:
         raise MacadamError("Could not find the gvproxy socket path.")
-
     return match.group(1)
 
 ...
 
 def get_ssh_port() -> int:
-    try:
-        result = run_command(["pgrep", "-af", "gvproxy"], capture_output=True)
-    except MacadamError as e:
-        raise MacadamError("The gvproxy process is not running.") from e
-
-    gvproxy_cmd = result.stdout.strip()
+    gvproxy_cmd = _get_gvproxy_cmdline()
     match = re.search(r'-ssh-port (\d+)', gvproxy_cmd)
     if not match:
         raise MacadamError("Could not find the ssh port from gvproxy.")
-
     return int(match.group(1))

Also applies to: 433-444

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@run-with-macadam.py` around lines 386 - 397, Extract the duplicated logic
that runs "pgrep -af gvproxy" and returns/parses the gvproxy command line into a
shared helper (e.g., a function named fetch_gvproxy_cmd or
get_gvproxy_process_cmd) and update get_gvproxy_socket_path and get_ssh_port to
call that helper; the helper should call run_command(["pgrep","-af","gvproxy"],
capture_output=True), raise or rewrap MacadamError with the same contextual
message when the command fails, and return the stdout string so
get_gvproxy_socket_path can run re.search(r'-services unix://([^ ]+)', ...) and
get_ssh_port can parse the port—this removes duplicated pgrep invocation and
centralizes error handling.

---

106-109: Stale/misleading default bundle path.

The hardcoded fallback crc_vfkit_4.19.3_arm64.crcbundle references a vfkit/arm64 bundle while CI downloads a libvirt/amd64 bundle (version 4.21.0). Although CI sets CRC_BUNDLE_PATH explicitly, this default is confusing and could silently break local dev runs on x86_64.

Suggested fix

     crc_bundle_path_str = os.environ.get(
         "CRC_BUNDLE_PATH",
-        str(Path.home() / "Downloads" / "crc_vfkit_4.19.3_arm64.crcbundle")
+        "",
     )
+    if not crc_bundle_path_str:
+        raise MacadamError("CRC_BUNDLE_PATH environment variable must be set.")
     crc_bundle_path = Path(crc_bundle_path_str)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@run-with-macadam.py` around lines 106 - 109, The default fallback for
CRC_BUNDLE_PATH (variable crc_bundle_path_str) is stale and points to an arm64
vfkit bundle; update the fallback to match CI's libvirt amd64 bundle or choose
per-host architecture: either change the hardcoded fallback string to
"crc_libvirt_4.21.0_amd64.crcbundle" in the Path.home()/Downloads path, or
replace the single hardcoded default with a small platform check
(platform.machine()) that selects the appropriate filename for "amd64" vs
"arm64" so local x86_64 runs don't silently use the wrong bundle; ensure the
variable name crc_bundle_path_str and the env var "CRC_BUNDLE_PATH" are
preserved.

ci.sh (1)

21-33: Remove commented-out code before merging.

Large blocks of commented-out code (old CRC workflow at lines 21–33 and 65–81) should be removed before this leaves WIP status. Git history preserves the old code.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@ci.sh` around lines 21 - 33, Remove the large commented-out CRC workflow
blocks (the block starting with the commented shellcheck/snс.sh lines and the
openshift-tests extraction plus createdisk.sh steps, and the other analogous
commented block later in the script) from ci.sh so only active code remains;
delete those commented lines entirely (do not reintroduce them as comments),
leaving any required environment exports or live commands intact, relying on git
history to retain the old workflow.