Refactor Keycloak direct-access-grants into shared helper; harden eval port-forwards; add cluster/dry flags to ibac comparison

[yoavkatz]

Summary

• Extract shared Keycloak helper. The "enable Direct Access Grants for the rossoctl client" logic was duplicated across deploy-agent.sh and deploy-benchmark.sh. It now lives in a new libsh/keycloak-direct-access.sh as enable_direct_access_grants, which resolves master-realm admin creds from env vars → the keycloak-initial-admin secret → admin/admin defaults, and exits with a clear diagnostic on any failure. An unreachable Keycloak (empty response) is now distinguished from a rejected credential.
• Fix delete-all-deployments.sh. Its token requests use grant_type=password against the rossoctl client, which requires Direct Access Grants to be enabled first. It now sources the helper and calls it before requesting a token.
• Harden evaluate-benchmark.sh port-forwards. Before binding a local port, detect a stale (or suspended) leftover kubectl port-forward holding it and clean it up — refusing to touch any non-kubectl process. Port-forward output is captured to a temp file so the real error is surfaced when startup fails; temp files are removed on cleanup.
• Harden analyze-run.sh / download_mlflow_traces.py MLflow auth. A distinct exit code 75 ("MLflow rejected the token") is raised via MLflowAuthError and handled at the analyze-run.sh call site, with urllib.error correctly imported. analyze-run.sh also authenticates with grant_type=password against the mlflow client (sourcing the shared helper), and gains a --save-traces flag to persist downloaded traces.
• Add cluster/dry flags to run-ibac-comparison.sh. New --kind / --openshift DOMAIN / --in-cluster cluster flags and a --dry run mode, all forwarded verbatim to deploy-and-evaluate.sh, delete-all-deployments.sh, and analyze-run.sh (which already validate and support them).

Testing

• bash -n syntax-checks pass on all changed scripts; py_compile clean on the Python.
• Verified downstream deploy-and-evaluate.sh accepts the forwarded --dry, --kind, --openshift, and --in-cluster flags.

Review fixes

• Forward CLUSTER_FLAG to the final analyze-run.sh comparison call (previously missing, so --openshift runs would target a Kind-style MLflow URL and fail after both benchmark runs completed).
• Distinguish an unreachable Keycloak (empty response) from a rejected credential in the helper's admin-token error diagnostic.

[kellyaa]

Tested this on a local Kind cluster (kind-rossoctl) against a live Keycloak. All four changes work as described, and the core fix is verifiably real. One bug worth fixing before merge, plus two minor notes.

The core fix is confirmed

I proved the delete-all-deployments.sh fix with a before/after against the same cluster state — set directAccessGrantsEnabled=false on the rossoctl client, then ran both versions:

• main's version → ⚠ Warning: Fetched password from cluster but authentication failed, exit 1
• this branch → enables the grant, obtains a token, exit 0

So the password-grant prerequisite really was broken on main, and this fixes it.

What else I verified

Keycloak helper — works against live Keycloak, and the flag genuinely persists (checked via the admin API, not just an HTTP 200 on the PUT). All three failure paths exit 1 with a useful diagnostic: wrong admin password, nonexistent client, unreachable Keycloak.

Port-forward hardening — both branches behave correctly:

• a real stale kubectl port-forward holding the port → detected, killed, port freed
• a non-kubectl process holding the port (a Python listener) → refused to kill it, exited 1 and printed the offending process. The safety guard does what it claims.

--dry / cluster flags — dry run completes and flags forward correctly. Confirmed delete-all-deployments.sh does accept --kind/--openshift, so that forwarding is valid.

Refactor fidelity — bash -n clean on all 7 shell scripts, py_compile clean on the Python. The extracted helper is a faithful superset of the two copies it replaces: it adopts deploy-benchmark.sh's stronger version, so deploy-agent.sh picks up credential resolution and real error handling it previously lacked (it used to silently swallow every failure). The single shellcheck warning in evaluate-benchmark.sh (SC2155) is pre-existing and unrelated.

Bug: analyze-run.sh doesn't receive the cluster flag

run-ibac-comparison.sh forwards CLUSTER_FLAG to delete-all-deployments.sh and deploy-and-evaluate.sh, but not to the final analyze-run.sh call. analyze-run.sh does accept cluster flags and defaults to kind when none is given (analyze-run.sh:146-148), so with --openshift DOMAIN the comparison step will target a Kind-style MLflow URL and fail — after both benchmark runs have already completed, which is the expensive way to find out.

Dry run with --openshift apps.mycluster.example.com shows the flag missing:

[DRY RUN] Would execute:
.../analyze-run.sh -c gsm8k-10-parallel-1-cejqm-ibac\,gsm8k-10-parallel-1-cejqm

One-line fix:

run_step "$SCRIPT_DIR/analyze-run.sh" -c "${EXPERIMENT_PLUGIN},${EXPERIMENT_BASE}" ${CLUSTER_FLAG[@]+"${CLUSTER_FLAG[@]}"}

This is invisible on kind, which is probably why it slipped through — the testing notes are syntax-checks only, no cluster run.

Minor notes

1. Misleading diagnostic when Keycloak is unreachable. A connection failure prints Set KEYCLOAK_ADMIN_PASSWORD in your .env..., blaming credentials for what is actually a network problem:

   Error: Could not obtain master-realm admin token from Keycloak
     Response:
     Set KEYCLOAK_ADMIN_PASSWORD in your .env if the master realm admin password is not 'admin'.
   

   Worth distinguishing an empty response (unreachable) from a rejected one (bad creds).

2. analyze-run.sh (+123/-26) and download_mlflow_traces.py aren't mentioned in the PR description — together that's the largest diff here. The change itself looks good: a distinct exit code 75 for "MLflow rejected the token", raised via MLflowAuthError and handled at analyze-run.sh:514, with urllib.error correctly imported. Worth adding to the summary so reviewers know to look at it.

One caveat on my testing

My keycloak-initial-admin secret happens to contain admin/admin*, so the secret-fallback path and the hardcoded admin/admin default are indistinguishable on this cluster. I confirmed the env-var override path works, but the secret-resolution path is not independently proven — that needs a cluster with a non-default admin password (e.g. a real RHBK deployment).

[clawgenti]

This PR introduces significant improvements to MLflow authentication, error handling, and script robustness within the A2A runner. It also adds better cluster deployment options. All checks pass. Ready for human review.

Reviewed by clawgenti using the github-pr-review skill

[clawgenti]

Solid refactor that eliminates meaningful code duplication and adds actionable error handling throughout. The --save-traces support, exit-75 auth-error path, and stale port-forward cleanup are all well-considered additions.

• Nit (analyze-run.sh): --save-traces|-save-traces accepts a single-dash multi-character alias that isn't documented in usage and is non-standard POSIX style — consider removing the -save-traces alternative or documenting it.
• Nit (libsh/keycloak-direct-access.sh): The Keycloak client PUT sends a partial body {"directAccessGrantsEnabled": true}; the Keycloak REST API expects the full client representation on a PUT (a PATCH-style partial update is not guaranteed). In practice it works for this flag, but it silently drops any fields not included — consider using PATCH if available, or fetching the full client config first and merging the flag in.

All checks pass. Ready for human review.

Reviewed by clawgenti using the github-pr-review skill

[clawgenti]

Nit: --save-traces|-save-traces — the single-dash -save-traces is a non-standard alias (POSIX single-dash flags are single-character). It's also not reflected in the usage() output or examples. Consider removing it, or if you want to keep it, document it.

[clawgenti]

Nit: Sending {"directAccessGrantsEnabled": true} as a bare PUT body is a partial-update pattern, but the Keycloak REST API treats PUT /clients/{id} as a full-replace. For this single boolean flag it works in practice, but any fields not included in the body will be silently reset. Worth either (a) fetching the current client representation first and merging the flag, or (b) noting this footgun explicitly in the comment.

[clawgenti]

Clean refactor — the shared helper removes real duplication and the port-forward hardening is solid. One correctness issue worth addressing before merge.

Findings:

• keycloak-direct-access.sh: The PUT to /admin/realms/rossoctl/clients/{id} sends only {"directAccessGrantsEnabled": true}. Keycloak's client PUT is a full replace — it will silently wipe all other client settings (redirect URIs, protocol mappers, scopes, etc.) not present in the payload. The helper needs to GET the existing client config first and merge in the flag before sending the PUT.
• Minor: evaluate-benchmark.sh captures PF_PROMETHEUS_LOG but there is no startup check that surfaces its contents on failure, unlike the OTEL collector path.

---

Reviewed by clawgenti using the github-pr-review skill

[clawgenti]

Bug: Keycloak PUT is a full replace, not a partial update.

Sending only {"directAccessGrantsEnabled": true} will silently wipe all other client settings (redirect URIs, protocol mappers, client scopes, etc.) that are not present in the payload.

The correct pattern:

local client_rep
client_rep=$(curl -s "$KEYCLOAK_API/admin/realms/rossoctl/clients/$client_id" \
    -H "Authorization: Bearer $admin_token" 2>/dev/null)
client_rep=$(echo "$client_rep" | jq '.directAccessGrantsEnabled = true')
# Then PUT $client_rep instead of the hardcoded fragment

[clawgenti]

This PR is a clean, well-considered refactor: the shared enable_direct_access_grants helper properly addresses the previous nits (full client PUT via jq merge, distinguishing unreachable Keycloak from bad creds). Two minor nits remain from the prior round.

• Nit (libsh/keycloak-direct-access.sh line 91): The PUT error response is captured into the hardcoded path /tmp/kc_put_response.txt. If the helper is ever called concurrently (or a leftover file exists from a previous run), this races/misleads. A mktemp temp file (cleaned up on exit) is safer.
• Nit (evaluate-benchmark.sh): Prometheus port-forward output is now captured to PF_PROMETHEUS_LOG, but unlike the OTEL collector path there is no ps -p $PF_PROMETHEUS_PID startup check that surfaces the log contents on failure. The asymmetry means a Prometheus startup failure silently passes the health wait.

---

Reviewed by clawgenti using the github-pr-review skill

[clawgenti]

Nit: /tmp/kc_put_response.txt is a hardcoded shared path — if two callers run concurrently, or a stale file exists from a prior failed invocation, the error message will reflect the wrong content. Consider local put_response_file; put_response_file=$(mktemp) and rm -f "$put_response_file" after use.

[clawgenti]

Nit: PF_PROMETHEUS_LOG is now captured but the startup ps -p check only exists for the OTEL collector. Without a corresponding ps -p $PF_PROMETHEUS_PID check here that prints $PF_PROMETHEUS_LOG on failure, a silently-crashed Prometheus port-forward will go undetected until a later step fails with a confusing error.