OSAC-757: Update keycloak deployment manifests to match fulfillment-service changes - #286
Conversation
|
@obochan-rh: This pull request references OSAC-757 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the sub-task to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
26a82d3 to
7ccbebc
Compare
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds an Changesosac-admin client and credential script update
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 1 warning)
✅ Passed checks (9 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
/retest |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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`:
- Line 18: The FC_CLIENT variable is unconditionally assigned on line 18, which
overwrites any externally provided value and prevents the fallback logic on line
148 from ever being used. Replace the unconditional assignment of FC_CLIENT with
bash parameter-default assignment syntax to set the default value to
"osac-controller" only when FC_CLIENT is not already set externally. This allows
external configuration to take precedence while providing a fallback default
when needed.
🪄 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: dc0b1bda-c9c5-425e-87ca-f2384b9458c6
📒 Files selected for processing (1)
scripts/refresh-after-snapshot.sh
|
/retest |
2 similar comments
|
/retest |
|
/retest |
|
/approve |
|
/cc @adriengentil |
|
🏗️ CI Triage: Root cause: The CI job failed to acquire a bare metal machine from the OFCIR pool because no resources of type 'osac' were available. Explanation: During the pre-flight phase, the Evidence: Suggestion: Retrigger the job later when bare metal machines become available in the pool. Prow job | Build For deeper investigation, use the |
|
/hold |
|
@obochan-rh I've set that pr on hold so we can test it properly. |
|
@eliorerz can we unhold it and run the test if needed ? |
|
/test ? |
Slash commandsCommands match workflow file names by prefix.
|
8bd36e9 to
c131b9d
Compare
c131b9d to
60a3bb2
Compare
|
/unhold |
|
/retest |
|
Re-triggered failed runs:
|
|
/retest |
|
Re-triggered failed runs:
|
60a3bb2 to
53af04b
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
scripts/refresh-after-snapshot.sh (1)
148-148: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDead fallback: jq branch can never execute.
FC_CLIENTis always set by Line 18's${FC_CLIENT:-"osac-controller"}, so the${FC_CLIENT:-$(jq ...)}fallback here is unreachable —FC_CLIENTis never empty at this point. Simplify to a direct assignment.♻️ Suggested simplification
- FC_CLIENT_ID=${FC_CLIENT:-$(jq -er 'first(.clients[] | select(.serviceAccountsEnabled==true)) | .clientId' "${REALM_JSON}")} + FC_CLIENT_ID="${FC_CLIENT}"🤖 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` at line 148, The FC_CLIENT_ID assignment in refresh-after-snapshot.sh uses an unreachable jq fallback because FC_CLIENT is already guaranteed to be set earlier via FC_CLIENT:-"osac-controller". Simplify the logic by removing the fallback branch and assign FC_CLIENT_ID directly from FC_CLIENT, keeping the surrounding setup unchanged.
🤖 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.
Duplicate comments:
In `@scripts/refresh-after-snapshot.sh`:
- Line 148: The FC_CLIENT_ID assignment in refresh-after-snapshot.sh uses an
unreachable jq fallback because FC_CLIENT is already guaranteed to be set
earlier via FC_CLIENT:-"osac-controller". Simplify the logic by removing the
fallback branch and assign FC_CLIENT_ID directly from FC_CLIENT, keeping the
surrounding setup unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 5853b476-467b-44e7-9336-0a2de8cc4686
⛔ Files ignored due to path filters (1)
prerequisites/keycloak/service/files/realm.jsonis excluded by!**/realm.json
📒 Files selected for processing (1)
scripts/refresh-after-snapshot.sh
|
/retest |
|
Re-triggered failed runs:
|
|
/retest |
|
Re-triggered failed runs:
|
Add the osac-admin Keycloak client and service account user to the installer's realm.json, matching the pattern established in fulfillment-service PR osac-project#462. - Add osac-admin confidential client with serviceAccountsEnabled - Add service-account-osac-admin user linked to the new client - Add roles.client["osac-admin"] entry required by Keycloak - Set osac-controller directAccessGrantsEnabled to false (service accounts should only use client credentials grant) Related: https://redhat.atlassian.net/browse/OSAC-757 Signed-off-by: Ofer Bochan <obochan@obochan-thinkpadp1gen7.rmtil.csb>
Hardcode osac-controller in the credential lookup instead of using a generic serviceAccountsEnabled select. The previous approach returns multiple results now that osac-admin is also a service account client, breaking the jq query. This matches setup.sh which already hardcodes osac-controller. Related: https://redhat.atlassian.net/browse/OSAC-757 Signed-off-by: Ofer Bochan <obochan@obochan-thinkpadp1gen7.rmtil.csb>
Address review feedback: instead of hardcoding osac-controller in the function body, use an FC_CLIENT variable (set at the top, with a jq fallback to first serviceAccountsEnabled client).
Use parameter-default assignment so an externally supplied FC_CLIENT is not unconditionally overwritten.
53af04b to
bd4eccf
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: CrystalChun, eliorerz, obochan-rh, oourfali 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 |
6 participants
Summary
Aligns the
osac-installerKeycloak realm configuration and supporting scripts with the service-account changes introduced in fulfillment-service PR #462.osac-adminclient & service-account user torealm.json— mirrors the new admin service-account client created in the fulfillment-service IT Helm chart. Also setsdirectAccessGrantsEnabled: falseonosac-controllerto match the client-credentials-only pattern.refresh-after-snapshot.sh— the oldselect(.serviceAccountsEnabled == true)now returns multiple clients; switched to explicitselect(.clientId == "osac-controller")so the credential secret is always resolved unambiguously.sync-authconfig-rego.py& resync overlay AuthConfig policies — the base Rego now listscontrollerafteradmininemergency_service_accounts, so the injection-point pattern was updated fromadmin→controller. Re-ran--fixto bring all four overlays in sync (addscontrollerSA, corrects comments, updates API endpoint names).Changed files
prerequisites/keycloak/service/files/realm.jsonosac-adminclient,service-account-osac-adminuser,roles.cliententry; setdirectAccessGrantsEnabled: falseonosac-controllerscripts/refresh-after-snapshot.shosac-controllerlookup instead of genericserviceAccountsEnabledqueryscripts/sync-authconfig-rego.pyadmintocontrolleroverlays/{caas-ci,development,osac-integration,vmaas-ci}/kustomization.yamlTest plan
python scripts/sync-authconfig-rego.pyexits 0 (no drift)jq . prerequisites/keycloak/service/files/realm.json > /dev/nullpasses (valid JSON)osac-adminandosac-controllerclients appear in the Keycloak admin consolerefresh-after-snapshot.shcorrectly recreates thefulfillment-controller-credentialssecret after a snapshot bootosac-controllerclient credentialsResolves: OSAC-757
Related: fulfillment-service PR #462
Summary by CodeRabbit
osacby refining admin roles and service-account setup, and disabling direct access grants for the controller while keeping service accounts enabled.