Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: omertuc 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 |
|
Caution Review failedThe pull request is closed. WalkthroughUpdates Containerfile base image digest and EXPOSE, mounts a CA bundle into the lightspeed-stack container, advances the lightspeed-stack submodule pointer, adds Authorization: Bearer ${OCM_TOKEN} to scripts/query.sh, and introduces JWT role_rules plus authorization access_rules in template.yaml. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User
participant C as Client (UI/Script)
participant LS as Lightspeed Stack
participant JWT as JWT Validator
participant AuthZ as Authorization Engine
U->>C: Request with JWT
C->>LS: HTTP request + Authorization: Bearer <JWT>
LS->>JWT: Validate token, extract claims
JWT-->>LS: Claims (org_id, is_internal, realm roles)
LS->>AuthZ: Apply role_rules → roles (e.g., redhat_employee)
AuthZ-->>LS: Permit or deny per access_rules
alt permitted
LS-->>C: 200 OK + response
else denied
LS-->>C: 403 Forbidden
end
sequenceDiagram
autonumber
participant Dev as Developer
participant Script as scripts/query.sh
participant API as /v1/models
Dev->>Script: run get_available_models (OCM_TOKEN set)
Script->>API: GET /v1/models with Authorization: Bearer ${OCM_TOKEN}
alt valid token
API-->>Script: 200 OK (models list)
else missing/invalid token
API-->>Script: 401/403 error
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (5)
✨ 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 (
|
![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
🧹 Nitpick comments (2)
Containerfile.assisted-chat (1)
7-7: EXPOSE port mismatch with deployment defaults.Service defaults to 8090 in template.yaml; EXPOSE 8080 may confuse operators/tools. Align to 8090 (or expose both if needed).
Apply:
-EXPOSE 8080 +EXPOSE 8090template.yaml (1)
178-192: Make authz intent explicit; remove confusing commented role block.The currently uncommented actions are part of redhat_employee (good), but the surrounding comment implies they belong to role "*". Clarify to avoid regressions.
authorization: access_rules: - 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 - - streaming_query - - get_conversation - - list_conversations - - delete_conversation - - feedback + - get_models + - query + - streaming_query + - get_conversation + - list_conversations + - delete_conversation + - feedback + # To allow all authenticated users in the future, add: + # - role: "*" + # actions: [get_models, query, streaming_query, get_conversation, list_conversations, delete_conversation, feedback]Ensure LIGHTSPEED_SERVICE_AUTH_ENABLED is set to true in the target environment; otherwise these rules won’t be enforced.
📜 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 ignored due to path filters (1)
config/ca-bundle.pemis excluded by!**/*.pem
📒 Files selected for processing (5)
Containerfile.assisted-chat(1 hunks)assisted-chat-pod.yaml(1 hunks)lightspeed-stack(1 hunks)scripts/query.sh(1 hunks)template.yaml(1 hunks)
⏰ 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)
lightspeed-stack (1)
1-1: Submodule bump to 691c83e: review auth changes & CI submodules
- Changed files in lightspeed-stack:
src/auth/jwk_token.py,src/authorization/resolvers.py— inspect for any breaking JWT/config/authz shifts.- Ensure your CI’s checkout step fetches submodules (e.g. GitHub Actions’
actions/checkoutwithsubmodules: trueor equivalent) so the lightspeed-stack update is pulled in.template.yaml (1)
168-176: Use JSON boolean true for is_internal;equalsis the correct operator.models/config.py defines JsonPathOperator = {"equals","contains","in"} and JwtRolesResolver uses direct equality (==) for EQUALS — a quoted "true" (string) will not match a JSON boolean claim.
Location: template.yaml (lines 168-176)
- - jsonpath: "$.is_internal" - operator: "equals" - value: "true" + - jsonpath: "$.is_internal" + operator: "equals" + value: true roles: ["redhat_employee"]Confirm JWT claim types: is_internal must be a JSON boolean; org_id claim type must match the elements in the
inlist (string vs number).assisted-chat-pod.yaml (1)
41-44: LGTM—fixes local dev trust store mount.Ensure ./config/ca-bundle.pem exists on the host; with subPath, missing file will fail container start.
Would you like a preflight script that checks presence/permissions of ./config/ca-bundle.pem before
oc apply?
| get_available_models() { | ||
| curl --silent --show-error -X 'GET' "${BASE_URL}/v1/models" -H 'accept: application/json' | ||
| curl --silent --show-error -X 'GET' "${BASE_URL}/v1/models" -H 'accept: application/json' -H "Authorization: Bearer ${OCM_TOKEN}" | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Harden token handling and HTTP error checks for models call.
With set -u, ${OCM_TOKEN} unbound will abort; also no status handling here unlike other calls.
Apply:
get_available_models() {
- curl --silent --show-error -X 'GET' "${BASE_URL}/v1/models" -H 'accept: application/json' -H "Authorization: Bearer ${OCM_TOKEN}"
+ # Ensure token present (and fetch if not)
+ if [[ -z "${OCM_TOKEN:-}" ]]; then
+ if ! get_ocm_token; then
+ echo "Failed to get OCM token for models"
+ return 1
+ fi
+ fi
+ tmpfile=$(mktemp)
+ status=$(curl --silent --show-error --output "$tmpfile" --write-out "%{http_code}" \
+ -H 'accept: application/json' \
+ -H "Authorization: Bearer ${OCM_TOKEN}" \
+ "${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"
}📝 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.
| get_available_models() { | |
| curl --silent --show-error -X 'GET' "${BASE_URL}/v1/models" -H 'accept: application/json' | |
| curl --silent --show-error -X 'GET' "${BASE_URL}/v1/models" -H 'accept: application/json' -H "Authorization: Bearer ${OCM_TOKEN}" | |
| } | |
| get_available_models() { | |
| # Ensure token present (and fetch if not) | |
| if [[ -z "${OCM_TOKEN:-}" ]]; then | |
| if ! get_ocm_token; then | |
| echo "Failed to get OCM token for models" | |
| return 1 | |
| fi | |
| fi | |
| tmpfile=$(mktemp) | |
| status=$(curl --silent --show-error --output "$tmpfile" --write-out "%{http_code}" \ | |
| -H 'accept: application/json' \ | |
| -H "Authorization: Bearer ${OCM_TOKEN}" \ | |
| "${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" | |
| } |
- Bump lsc to dev-20250828-691c83e - Fix annoying local dev /etc/tls/ca-bundle.pem issue - Fix query.sh to use OCM token for model listing - Bring back authz - taken from rh-ecosystem-edge#160
2 participants
Summary by CodeRabbit
New Features
Chores