Bump lightspeed-stack to authorization commit #152
Conversation
WalkthroughUpdates Containerfile.assisted-chat base image (tag + digest), removes build-time patching steps, advances lightspeed-stack submodule, adds an OCM token pre-check in scripts/query.sh’s new-conversation path, and adds JWT role mapping plus authorization access rules in template.yaml. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User
participant Q as scripts/query.sh
participant O as OCM
participant L as Lightspeed API
U->>Q: Start new conversation
Q->>O: Request OCM token
alt Token fetched
O-->>Q: Token
Q->>L: get_available_models (with token)
L-->>Q: Models
Q->>L: query/streaming_query (with token, selected model)
L-->>Q: Response
else Token failure
Q-->>U: "Failed to get OCM token for query" (exit 1)
end
sequenceDiagram
autonumber
participant C as Client
participant G as Gateway/API
participant J as JWT Role Mapper
participant A as Authorization Engine
participant S as Lightspeed Service
C->>G: Request with JWT
G->>J: Extract roles (jsonpath $.realm_access.roles[*])
J-->>G: Internal roles (e.g., redhat_employee)
G->>A: Check access_rules for action
alt Allowed (redhat_employee)
A-->>G: Permit
G->>S: Perform action
S-->>G: Result
G-->>C: Success
else Denied (nobody or not listed)
A-->>G: Deny
G-->>C: Forbidden
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–30 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: maorfr, omertuc The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/query.sh (1)
29-31: /v1/models now requires auth; add Authorization header and handle HTTP errors.You pre-check OCM login, but
get_available_modelsdoesn’t send the bearer token nor validate HTTP status. With auth enabled, this will 401 and later jq/fzf will fail underset -e. Include the header and return an error on non-2xx.-get_available_models() { - curl --silent --show-error -X 'GET' "${BASE_URL}/v1/models" -H 'accept: application/json' -} +get_available_models() { + local tmpfile status body + tmpfile=$(mktemp) + status=$(curl --silent --show-error --output "$tmpfile" --write-out "%{http_code}" \ + -H "Authorization: Bearer ${OCM_TOKEN}" \ + -H 'accept: application/json' \ + -X 'GET' "${BASE_URL}/v1/models") + body=$(cat "$tmpfile") + rm "$tmpfile" + if ! good_http_response "$status"; then + echo "Error: Failed to fetch models (HTTP $status)" + echo "Response: $body" + return 1 + fi + echo "$body" +}
🧹 Nitpick comments (2)
scripts/query.sh (1)
223-223: Nit: error message scope.The message reads "Failed to get OCM token for query" but this block is for model listing during new conversation. Consider clarifying to "for model listing" for consistency with history/listing blocks.
template.yaml (1)
174-190: Clarify the comment placement to avoid misreading the action scope.The commented-out wildcard example sits between
actions:and the rest of the un-commented actions. It reads as if the actions following it belong to the wildcard block, while they actually continue theredhat_employeeallowlist. Move the example above the block or reword to avoid confusion.- - role: redhat_employee - actions: - - get_models - # Temporarily we only want redhat employees to be able to use the service, - # uncomment when we want to allow all authenticated users - # - role: "*" - # actions: - - query + # Temporarily we only want redhat employees to be able to use the service. + # When we want to allow all authenticated users, add a new rule: + # - role: "*" + # actions: + # - get_models + # - query + # - streaming_query + # ... + - role: redhat_employee + actions: + - get_models + - query - streaming_query - get_conversation - list_conversations - delete_conversation - feedback - get_metrics - info
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
Containerfile.assisted-chat(1 hunks)lightspeed-stack(1 hunks)scripts/query.sh(1 hunks)template.yaml(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
scripts/query.sh (1)
utils/ocm-token.sh (1)
get_ocm_token(9-25)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Red Hat Konflux / assisted-chat-saas-main-on-pull-request
🔇 Additional comments (3)
Containerfile.assisted-chat (1)
2-3: Manual verification required: confirm tag⇄digest match
Automated checks in this environment couldn’t resolve thedev-20250826-969904btag’s digest. Please verify locally that it matchessha256:5582703f6220f0668ea465c66e81f2a5a425437e1a12b4419c8a61e930a11aadbefore merging:• Using skopeo (requires skopeo + jq):
skopeo inspect docker://quay.io/lightspeed-core/lightspeed-stack:dev-20250826-969904b \ | jq -r '.Digest'• Or using Docker CLI:
docker pull quay.io/lightspeed-core/lightspeed-stack:dev-20250826-969904b docker inspect --format='{{index .RepoDigests 0}}' quay.io/lightspeed-core/lightspeed-stack:dev-20250826-969904bEnsure the returned digest equals the pinned value above.
template.yaml (2)
56-58: Auth defaults to false; ensure it’s enabled where this template is applied.PR intent is to restrict usage to Red Hat employees. With
LIGHTSPEED_SERVICE_AUTH_ENABLEDdefaulting to "false", deployments that don’t override this param won’t enforce JWT/RBAC. Confirm the parameter is set to "true" in the environments that consume this template (e.g., via Qontract params or overlays).Also applies to: 154-155
168-173: Please verify the exact role claim emitted by your Identity ProviderI only found the plural form (“redhat:employees”) in
template.yaml(line 171) and no occurrences of the singular form in the codebase. We can’t confirm from code alone which variant your SSO will emit.• Confirm with your SSO/Keycloak what exact string (and casing) appears under
$.realm_access.roles[*]in the JWT at runtime.
• If you discover tokens carrying “redhat:employee” instead of (or in addition to) “redhat:employees”, add a secondrole_rulesentry for the singular form:jwt_configuration: user_id_claim: ${USER_ID_CLAIM} username_claim: ${USERNAME_CLAIM} role_rules: - jsonpath: "$.realm_access.roles[*]" operator: "contains" value: "redhat:employees" roles: ["redhat_employee"] + - jsonpath: "$.realm_access.roles[*]" + operator: "contains" + value: "redhat:employee" + roles: ["redhat_employee"]Optionally, if your template engine supports regex or case-insensitive matching, you could consolidate both in one rule to avoid duplication.
lightspeed-stack
Outdated
| @@ -1 +1 @@ | |||
| Subproject commit 053d49fb6e19c63cd51e77816f7c7cb5907d0f93 | |||
| Subproject commit 969904b4aef1dbe4379b279effe065237233a687 | |||
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Submodule bump acknowledged; please verify downstream integration (image, auth rules, script pre-check).
The pointer change itself is fine. Given this bump gates access to Red Hat employees via the new authorization features, please validate that all consuming artifacts in this PR match the bumped commit and intended policies.
Run this script from repo root to confirm consistency across the PR (submodule pointer, base image tag/digest, template auth config, and the OCM token pre-check in scripts/query.sh):
If helpful, I can add a smoke test plan to validate:
- Unauthorized user receives a clear 403 with a human-friendly message.
- A JWT with realm_access.roles containing “redhat:Employees” maps to role “redhat_employee” and can: query, stream, list/get/delete own conversations, and submit feedback.
- “get_config” and all “other_conversations” operations are denied to non-admins.
🏁 Script executed:
#!/bin/bash
set -euo pipefail
EXPECTED_SUBMOD_SHA=969904b4aef1dbe4379b279effe065237233a687
EXPECTED_IMAGE_TAG='dev-20250826-969904b'
EXPECTED_IMAGE_DIGEST='sha256:5582703f6220f0668ea465c66e81f2a5a425437e1a12b4419c8a61e930a11aad'
echo "Verifying submodule pointer…"
actual_sha="$(git ls-tree HEAD lightspeed-stack | awk '{print $3}')"
echo "lightspeed-stack @ ${actual_sha}"
test "$actual_sha" = "$EXPECTED_SUBMOD_SHA" || { echo "ERROR: Submodule SHA mismatch"; exit 1; }
echo "Verifying Containerfile base image tag/digest…"
fd -a 'Containerfile.assisted-chat' -t f | while read -r f; do
rg -n "${EXPECTED_IMAGE_TAG}" "$f" >/dev/null || { echo "ERROR: Tag not updated in $f"; exit 1; }
rg -n "${EXPECTED_IMAGE_DIGEST}" "$f" >/dev/null || { echo "ERROR: Digest not updated in $f"; exit 1; }
done
echo "Verifying authorization and role mapping in template.yaml…"
fd -a 'template.yaml' -t f | while read -r f; do
rg -n 'authorization:\s*$' "$f" -C2 || { echo "ERROR: authorization block missing in $f"; exit 1; }
rg -n '\$\.realm_access\.roles\[\*\]' "$f" >/dev/null || { echo "ERROR: role_rules jsonpath missing in $f"; exit 1; }
rg -n 'redhat:Employees' "$f" >/dev/null || { echo "ERROR: expected role value missing in $f"; exit 1; }
rg -n 'roles:\s*\[\s*redhat_employee\s*\]' "$f" >/dev/null || { echo "ERROR: role mapping to redhat_employee missing in $f"; exit 1; }
rg -n 'access_rules:' "$f" >/dev/null || { echo "ERROR: access_rules missing in $f"; exit 1; }
done
echo "Verifying OCM token pre-check in scripts/query.sh…"
fd -a 'scripts/query.sh' -t f | while read -r f; do
rg -n 'Failed to get OCM token for query' "$f" >/dev/null || { echo "ERROR: OCM token pre-check message missing in $f"; exit 1; }
done
echo "All checks passed ✅"Length of output: 1694
Mismatch in template.yaml role value causing verification failure
The automated verification script failed when validating the role value in template.yaml. The file currently uses a lowercase role identifier, which does not match the expected value:
- File: template.yaml
- Current line:
value: "redhat:employees" - Expected:
value: "redhat:Employees"
Please update the role value to "redhat:Employees" (note the uppercase “E”) so that it aligns with the authorization rules and allows the downstream checks to pass. Once updated, rerun the provided script to confirm:
# After updating template.yaml:
bash verify-integration.shIf you’d like, I can also help draft a smoke-test plan to ensure:
- Unauthorized users receive a clear 403 response.
- JWTs with
realm_access.rolescontaining “redhat:Employees” map correctly toredhat_employee. - Only the intended operations are permitted for this role (query, stream, list/get/delete own conversations, submit feedback).
- Non-admins are denied “get_config” and “other_conversations” operations.
🤖 Prompt for AI Agents
In lightspeed-stack template.yaml around line 1, the role value is using the
wrong case ("redhat:employees"); update the value to "redhat:Employees" (capital
E) so it matches the expected authorization rule; save the file and rerun bash
verify-integration.sh to confirm the verification passes.
|
|
||
| if ! get_ocm_token; then | ||
| echo "Failed to get OCM token for query" | ||
| return 1 | ||
| fi | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Top-level 'return' will abort with a confusing bash error; use 'exit 1' instead.
This block runs at the top level (not inside a function). Using 'return' here emits "bash: return: can only return' from a function or sourced script" and exits due to set -e, but the error is noisy and misleading. Replace with exit 1`.
- if ! get_ocm_token; then
- echo "Failed to get OCM token for query"
- return 1
- fi
+ if ! get_ocm_token; then
+ echo "Failed to get OCM token for model listing"
+ exit 1
+ fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if ! get_ocm_token; then | |
| echo "Failed to get OCM token for query" | |
| return 1 | |
| fi | |
| if ! get_ocm_token; then | |
| echo "Failed to get OCM token for model listing" | |
| exit 1 | |
| fi |
🤖 Prompt for AI Agents
In scripts/query.sh around lines 221 to 226, the script uses "return 1" at
top-level which causes a confusing "can only `return' from a function or sourced
script" error; replace the top-level "return 1" with "exit 1" so the script
terminates cleanly when get_ocm_token fails, ensuring consistent exit behavior
under set -e.
|
need to remove the patch? |
Not yet, LSC need to bump llama-stack first |
at least the LSC patch, not the llama-stack patch. |
| jwt_configuration: | ||
| user_id_claim: ${USER_ID_CLAIM} | ||
| username_claim: ${USERNAME_CLAIM} | ||
| role_rules: |
There was a problem hiding this comment.
Let's move this to a parameter so we can tweak it from app interface
| operator: "contains" | ||
| value: "redhat:employees" | ||
| roles: ["redhat_employee"] | ||
| authorization: |
|
Let's merge the bump first, then tweak the configs / parameters later |
|
/retest |
1 similar comment
|
/retest |
Bump lightspeed-stack submodule to include the authorization changes from lightspeed-core/lightspeed-stack#356 Use the new authorization features to restrict access to the assisted-chat service to, for now, Red Hat employees only. In the future we can open this up to all authenticated users. Also bumped to a version that no longer requires the patching of llama-stack / lightspeed-stack to support postgres SSL, so removed the patching steps from the Containerfile.
|
/lgtm |
|
/test eval-test |
|
/retest |
|
@omertuc: The following test failed, say
Full PR test history. Your PR dashboard. 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 kubernetes-sigs/prow repository. I understand the commands that are listed here. |
3 participants
See commit message
Summary by CodeRabbit