NO_ISSUE: parallelize refresh-after-snapshot for faster boot - #164
openshift-merge-bot[bot] merged 1 commit into
Conversation
|
Warning Review limit reached
More reviews will be available in 15 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository: osac-project/coderabbit/.coderabbit.yaml Review profile: ASSERTIVE Plan: Enterprise Run ID: 📒 Files selected for processing (1)
WalkthroughThe script refactors the post-snapshot refresh into callable functions and parallelizes Keycloak realm sync, fulfillment-credentials creation, kustomize overlay apply, TLS stabilization, fulfillment rollouts, and AAP configuration/readiness with coordinated waits. Security note: admin tokens and secrets are handled in background jobs—if uncaught failures occur, credential state could diverge (High severity; may leave services with stale/missing auth). ChangesRefresh Workflow Parallelization
Sequence Diagram(s)Sequence diagrams are embedded in the hidden review stack artifact above for reference. Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 2 warnings)
✅ Passed checks (8 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
5e90355 to
9ffa34b
Compare
omer-vishlitzky
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/refresh-after-snapshot.sh`:
- Around line 130-131: The current client selection uses serviceAccountsEnabled
and can return multiple clients; update the jq selectors so FC_CLIENT_ID and
FC_CLIENT_SECRET explicitly target the osac-controller client to match upstream
contract and README: use a jq filter that selects .clients[] | select(.clientId
== "osac-controller") to set FC_CLIENT_ID and then extract its .secret (or
empty) into FC_CLIENT_SECRET using REALM_JSON; ensure both assignments reference
the same explicit client-id "osac-controller" so multiple
service-account-enabled clients won't cause silent wrong selection.
- Around line 85-87: Replace hardcoded admin credentials by reading
KC_ADMIN_USER and KC_ADMIN_PASS from environment or a mounted secret file (fail
early if missing), then send them to Keycloak via curl using stdin (so
credentials are not visible in the process command line). Concretely: ensure
KC_ADMIN_USER/KC_ADMIN_PASS are loaded, validate they are non-empty, build the
form body (client_id=admin-cli&username=...&password=...&grant_type=password)
with printf or a heredoc and pipe it into curl using --data-binary `@-` (targeting
the same "${KC_URL}/realms/master/protocol/openid-connect/token") and capture
the access token into KC_ADMIN_TOKEN as before; keep the existing token
null/empty check and error out if retrieval fails.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 307ec28e-a806-4d96-a7e4-9fae3d1f21d6
📒 Files selected for processing (1)
scripts/refresh-after-snapshot.sh
| FC_CLIENT_ID=$(jq -er '.clients[] | select(.serviceAccountsEnabled == true) | .clientId' "${REALM_JSON}") | ||
| FC_CLIENT_SECRET=$(jq -er ".clients[] | select(.clientId == \"${FC_CLIENT_ID}\") | .secret // empty" "${REALM_JSON}") |
There was a problem hiding this comment.
Correctness Risk (MEDIUM): Client selection logic differs from upstream contract.
The upstream scripts/setup.sh explicitly selects osac-controller:
FC_CLIENT_SECRET=$(jq -er '.clients[] | select(.clientId == "osac-controller") | .secret // empty' ...)This implementation selects by serviceAccountsEnabled == true, which could match multiple clients. If multiple service-account-enabled clients exist in realm.json, jq outputs multiple lines and only the last clientId is captured—potentially selecting the wrong client silently.
The documented contract in overlays/osac-integration/README.md also specifies client-id=osac-controller.
🛡️ Proposed fix to match upstream contract
create_fulfillment_credentials() {
echo "[2/8] Recreating fulfillment controller credentials..."
- FC_CLIENT_ID=$(jq -er '.clients[] | select(.serviceAccountsEnabled == true) | .clientId' "${REALM_JSON}")
- FC_CLIENT_SECRET=$(jq -er ".clients[] | select(.clientId == \"${FC_CLIENT_ID}\") | .secret // empty" "${REALM_JSON}")
+ FC_CLIENT_ID="osac-controller"
+ FC_CLIENT_SECRET=$(jq -er '.clients[] | select(.clientId == "osac-controller") | .secret // empty' "${REALM_JSON}")
[[ -n "${FC_CLIENT_SECRET}" ]] || { echo "ERROR: Could not resolve secret for ${FC_CLIENT_ID} in realm.json" >&2; exit 1; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/refresh-after-snapshot.sh` around lines 130 - 131, The current client
selection uses serviceAccountsEnabled and can return multiple clients; update
the jq selectors so FC_CLIENT_ID and FC_CLIENT_SECRET explicitly target the
osac-controller client to match upstream contract and README: use a jq filter
that selects .clients[] | select(.clientId == "osac-controller") to set
FC_CLIENT_ID and then extract its .secret (or empty) into FC_CLIENT_SECRET using
REALM_JSON; ensure both assignments reference the same explicit client-id
"osac-controller" so multiple service-account-enabled clients won't cause silent
wrong selection.
There was a problem hiding this comment.
ci only, irrelevant
There was a problem hiding this comment.
Acknowledged — noted that create_fulfillment_credentials() in scripts/refresh-after-snapshot.sh is CI-only, so the multi-client selection risk via serviceAccountsEnabled == true is acceptable in that context.
(╯°□°）╯︵ 🐇
✏️ Learnings added
Learnt from: omer-vishlitzky
Repo: osac-project/osac-installer PR: 164
File: scripts/refresh-after-snapshot.sh:130-131
Timestamp: 2026-05-25T16:08:22.856Z
Learning: In `scripts/refresh-after-snapshot.sh` (osac-project/osac-installer), the `create_fulfillment_credentials()` function that selects the fulfillment client via `jq -er '.clients[] | select(.serviceAccountsEnabled == true)'` is used in CI only. The risk of it matching multiple service-account-enabled clients is considered acceptable and not a concern to flag in code review.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
|
/retest |
|
/hold |
|
/retest |
9ffa34b to
a27c9a2
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/refresh-after-snapshot.sh`:
- Around line 105-118: The user upsert loop in scripts/refresh-after-snapshot.sh
silently ignores failures because the POST/PUT curl calls for each USER_JSON are
not checking HTTP response codes; modify the loop that iterates over
USER_JSON/USER_UUID so that after each curl PUT to
"${KC_URL}/admin/realms/osac/users/${USER_UUID}" and after each curl POST to
"${KC_URL}/admin/realms/osac/users" you capture the HTTP status (like
HTTP_CODE=$(curl -sk -o /dev/null -w "%{http_code}" ...)), validate it against
expected success codes (e.g., 200/204 for PUT, 201 for POST), and on non-success
log a clear error including USERNAME/USER_UUID/HTTP_CODE and either exit with a
non-zero status or collect failures and fail at the end; ensure you still print
the success messages ("Updated user" / "Created user") only when the response
indicates success.
- Around line 89-103: The PUT/POST curl invocations for syncing clients (code
handling CLIENT_JSON/CLIENT_UUID using KC_ADMIN_TOKEN and KC_URL) swallow
responses and always echo "Updated/Created client" even on 4xx/5xx; change the
upsert logic to capture HTTP status and response body from each curl (for both
the PUT to "${KC_URL}/admin/realms/osac/clients/${CLIENT_UUID}" and the POST to
"${KC_URL}/admin/realms/osac/clients"), check that the status code is a 2xx
before printing success, and on non-2xx log the status, response body and client
id (CID) and either retry or exit with non-zero to fail the script so
configuration errors aren’t silently ignored.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: f0ff268f-805c-4542-8df4-4d3df1830df4
📒 Files selected for processing (1)
scripts/refresh-after-snapshot.sh
| echo " Syncing clients and users via admin API..." | ||
| jq -c '.clients[] | select(.protocol == "openid-connect" and .publicClient != true and .bearerOnly != true)' "${REALM_JSON}" | while IFS= read -r CLIENT_JSON; do | ||
| CID=$(echo "${CLIENT_JSON}" | jq -r '.clientId') | ||
| CLIENT_UUID=$(echo "${CLIENT_JSON}" | jq -r '.id') | ||
| HTTP_CODE=$(curl -sk -o /dev/null -w "%{http_code}" -H "Authorization: Bearer ${KC_ADMIN_TOKEN}" "${KC_URL}/admin/realms/osac/clients/${CLIENT_UUID}") | ||
| if [[ "${HTTP_CODE}" == "200" ]]; then | ||
| curl -sk -X PUT -H "Authorization: Bearer ${KC_ADMIN_TOKEN}" -H "Content-Type: application/json" \ | ||
| "${KC_URL}/admin/realms/osac/clients/${CLIENT_UUID}" -d "${CLIENT_JSON}" >/dev/null | ||
| echo " Updated client: ${CID}" | ||
| else | ||
| curl -sk -X POST -H "Authorization: Bearer ${KC_ADMIN_TOKEN}" -H "Content-Type: application/json" \ | ||
| "${KC_URL}/admin/realms/osac/clients" -d "${CLIENT_JSON}" >/dev/null | ||
| echo " Created client: ${CID}" | ||
| fi | ||
| done |
There was a problem hiding this comment.
Reliability Risk (MEDIUM): Silent failure on client upsert operations.
The PUT/POST curl calls redirect output to /dev/null and don't validate HTTP response codes. If Keycloak returns a 4xx/5xx error, the script logs "Updated/Created client" anyway and continues, potentially leaving client configuration out of sync.
Impact: Downstream services may fail to authenticate if client sync silently fails.
🛡️ Proposed fix to validate upsert responses
if [[ "${HTTP_CODE}" == "200" ]]; then
- curl -sk -X PUT -H "Authorization: Bearer ${KC_ADMIN_TOKEN}" -H "Content-Type: application/json" \
- "${KC_URL}/admin/realms/osac/clients/${CLIENT_UUID}" -d "${CLIENT_JSON}" >/dev/null
- echo " Updated client: ${CID}"
+ PUT_CODE=$(curl -sk -o /dev/null -w "%{http_code}" -X PUT -H "Authorization: Bearer ${KC_ADMIN_TOKEN}" -H "Content-Type: application/json" \
+ "${KC_URL}/admin/realms/osac/clients/${CLIENT_UUID}" -d "${CLIENT_JSON}")
+ [[ "${PUT_CODE}" =~ ^2 ]] || { echo "ERROR: Failed to update client ${CID} (HTTP ${PUT_CODE})" >&2; exit 1; }
+ echo " Updated client: ${CID}"
else
- curl -sk -X POST -H "Authorization: Bearer ${KC_ADMIN_TOKEN}" -H "Content-Type: application/json" \
- "${KC_URL}/admin/realms/osac/clients" -d "${CLIENT_JSON}" >/dev/null
+ POST_CODE=$(curl -sk -o /dev/null -w "%{http_code}" -X POST -H "Authorization: Bearer ${KC_ADMIN_TOKEN}" -H "Content-Type: application/json" \
+ "${KC_URL}/admin/realms/osac/clients" -d "${CLIENT_JSON}")
+ [[ "${POST_CODE}" =~ ^2 ]] || { echo "ERROR: Failed to create client ${CID} (HTTP ${POST_CODE})" >&2; exit 1; }
echo " Created client: ${CID}"
fi🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/refresh-after-snapshot.sh` around lines 89 - 103, The PUT/POST curl
invocations for syncing clients (code handling CLIENT_JSON/CLIENT_UUID using
KC_ADMIN_TOKEN and KC_URL) swallow responses and always echo "Updated/Created
client" even on 4xx/5xx; change the upsert logic to capture HTTP status and
response body from each curl (for both the PUT to
"${KC_URL}/admin/realms/osac/clients/${CLIENT_UUID}" and the POST to
"${KC_URL}/admin/realms/osac/clients"), check that the status code is a 2xx
before printing success, and on non-2xx log the status, response body and client
id (CID) and either retry or exit with non-zero to fail the script so
configuration errors aren’t silently ignored.
| jq -c '.users[]?' "${REALM_JSON}" | while IFS= read -r USER_JSON; do | ||
| USERNAME=$(echo "${USER_JSON}" | jq -r '.username') | ||
| USER_UUID=$(echo "${USER_JSON}" | jq -r '.id') | ||
| HTTP_CODE=$(curl -sk -o /dev/null -w "%{http_code}" -H "Authorization: Bearer ${KC_ADMIN_TOKEN}" "${KC_URL}/admin/realms/osac/users/${USER_UUID}") | ||
| if [[ "${HTTP_CODE}" == "200" ]]; then | ||
| curl -sk -X PUT -H "Authorization: Bearer ${KC_ADMIN_TOKEN}" -H "Content-Type: application/json" \ | ||
| "${KC_URL}/admin/realms/osac/users/${USER_UUID}" -d "${USER_JSON}" >/dev/null | ||
| echo " Updated user: ${USERNAME}" | ||
| else | ||
| curl -sk -X POST -H "Authorization: Bearer ${KC_ADMIN_TOKEN}" -H "Content-Type: application/json" \ | ||
| "${KC_URL}/admin/realms/osac/users" -d "${USER_JSON}" >/dev/null | ||
| echo " Created user: ${USERNAME}" | ||
| fi | ||
| done |
There was a problem hiding this comment.
Reliability Risk (MEDIUM): Silent failure on user upsert operations.
Same issue as client sync above—PUT/POST calls don't validate HTTP status codes. Failed user syncs could leave user state inconsistent, affecting login or authorization flows.
Apply the same response-code validation pattern as recommended for client upserts.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/refresh-after-snapshot.sh` around lines 105 - 118, The user upsert
loop in scripts/refresh-after-snapshot.sh silently ignores failures because the
POST/PUT curl calls for each USER_JSON are not checking HTTP response codes;
modify the loop that iterates over USER_JSON/USER_UUID so that after each curl
PUT to "${KC_URL}/admin/realms/osac/users/${USER_UUID}" and after each curl POST
to "${KC_URL}/admin/realms/osac/users" you capture the HTTP status (like
HTTP_CODE=$(curl -sk -o /dev/null -w "%{http_code}" ...)), validate it against
expected success codes (e.g., 200/204 for PUT, 201 for POST), and on non-success
log a clear error including USERNAME/USER_UUID/HTTP_CODE and either exit with a
non-zero status or collect failures and fail at the end; ensure you still print
the success messages ("Updated user" / "Created user") only when the response
indicates success.
|
/hold |
a27c9a2 to
e012f36
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: danmanor, jhernand, omer-vishlitzky The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/unhold |
3 participants
Summary
Parallelize the refresh-after-snapshot script to reduce boot time from ~10 minutes to ~2 minutes.
Same logical steps, same error handling, just concurrent where dependencies allow.
Test plan
Summary by CodeRabbit