parameterize lightspeed-stack auth rules#163
parameterize lightspeed-stack auth rules#163openshift-merge-bot[bot] merged 3 commits intorh-ecosystem-edge:mainfrom
Conversation
WalkthroughAdds two new template parameters (AUTHN_ROLE_RULES, AUTHZ_ACCESS_RULES) and replaces inlined authentication role rules and authorization access rules in the lightspeed-stack ConfigMap with parameter references; defaults are empty lists. No other logic changes. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
✨ 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 (
|
eranco74
left a comment
eranco74
left a comment
There was a problem hiding this comment.
LGTM
Did you test it locally?
- We need a CI job to cover the local development
![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 (3)
lightspeed-stack (1)
1-1: Confirm submodule bump reproducibility
The lightspeed-stack submodule is pinned to commit 969904b4aef1dbe4379b279effe065237233a687, which has no exact upstream tag. Add a one-liner in the PR description linking to the upstream commit (or release notes) for context. If possible, switch to a stable release tag for future reproducibility.assisted-service-mcp (1)
1-1: Optional: consider pinning to a tag or branch for traceability.If this commit corresponds to a release, referencing an annotated tag (or documenting the upstream release/PR) improves provenance and rollbacks.
template.yaml (1)
137-142: Document expected format for new params (JSON array) to avoid YAML shape issues.These values are injected inline. To remain valid YAML, callers should pass JSON-encoded lists (e.g., [] or [{"effect":"allow",...}]). Make that explicit.
Apply this diff to clarify parameter descriptions:
- name: AUTHN_ROLE_RULES value: '[]' - description: "lightspeed-stack authentication role rules" + description: "lightspeed-stack authentication role rules (JSON array string, e.g. [] or [{...}])" - name: AUTHZ_ACCESS_RULES value: '[]' - description: "lightspeed-stack authorization access rules" + description: "lightspeed-stack authorization access rules (JSON array string, e.g. [] or [{...}])"
📜 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 (5)
assisted-service-mcp(1 hunks)lightspeed-stack(1 hunks)llama-stack(1 hunks)scripts/build-images.sh(1 hunks)template.yaml(2 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 (4)
assisted-service-mcp (1)
1-1: Submodule bump to 2beef39…: pointers and CI checkout
Confirm assisted-service-mcp is at commit 2beef39f0bd15cb3232f840181585a17bbad8d7d (e.g. viagit submodule status), verify your lightspeed-stack and llama-stack submodules point to the commits containing the auth-parameterization changes, and ensure your CI pipeline runsgit submodule update --init --recursivellama-stack (2)
1-1: LGTM: metadata-only submodule bump.Pointer update only; no local code impact here.
1-1: Cross-repo compatibility check.This PR parameterizes auth rules; please confirm this llama-stack revision is compatible with those parameters (no breaking config schema/API drift).
template.yaml (1)
174-176: Confirm rendering with multi-rule payloads and auth_enabled=true.When LIGHTSPEED_SERVICE_AUTH_ENABLED=true but rules are [], effective behavior may deny everything depending on stack defaults. Also verify app-interface provides compact JSON so this line stays valid YAML.
I can generate a quick validation script that renders this template with sample JSON arrays and runs yamllint on the resulting ConfigMap to ensure correctness—want me to add it?
llama-stack
Outdated
| @@ -1 +1 @@ | |||
| Subproject commit 739b18edf8755390cd75b819852090de3be940c3 | |||
| Subproject commit eed25fc6e4f7e62c346a9d4ce5c7083e6500b7dd | |||
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify submodule commit exists and matches intent.
Confirm the hash resolves on the remote and (optionally) initialize to that exact commit.
🏁 Script executed:
#!/bin/bash
set -euo pipefail
HASH="eed25fc6e4f7e62c346a9d4ce5c7083e6500b7dd"
URL="$(git config -f .gitmodules --get submodule.llama-stack.url)"
# 1) Ensure the recorded tree entry matches the expected hash
git ls-tree HEAD llama-stack | awk '{print $3}' | tee /dev/stderr | grep -x "$HASH"
# 2) Ensure the hash exists on the remote
git ls-remote "$URL" | awk '{print $1}' | grep -x "$HASH"
# Optional: init and verify working tree is at the pinned commit
git submodule update --init --depth=1 llama-stack
git -C llama-stack rev-parse HEAD | tee /dev/stderr | grep -x "$HASH"Length of output: 389
🏁 Script executed:
#!/bin/bash
set -euo pipefail
HASH="eed25fc6e4f7e62c346a9d4ce5c7083e6500b7dd"
# 1) Load submodule URL
if [ -f .gitmodules ]; then
URL=$(git config -f .gitmodules --get submodule.llama-stack.url)
else
echo "ERROR: .gitmodules not found" >&2
exit 1
fi
echo "Submodule URL: $URL"
# 2) Verify recorded tree entry matches expected hash
TREE_HASH=$(git ls-tree HEAD llama-stack | awk '{print $3}')
echo "Tree entry hash: $TREE_HASH"
if [ "$TREE_HASH" != "$HASH" ]; then
echo "ERROR: Recorded tree hash does not match expected $HASH" >&2
exit 1
fi
# 3) Verify hash exists on remote
if ! git ls-remote "$URL" | awk '{print $1}' | grep -xq "$HASH"; then
echo "ERROR: Hash $HASH not found on remote $URL" >&2
exit 1
fi
# 4) Initialize submodule and verify working tree
git submodule update --init --depth=1 llama-stack
SUBMODULE_HEAD=$(git -C llama-stack rev-parse HEAD)
echo "Submodule HEAD commit: $SUBMODULE_HEAD"
if [ "$SUBMODULE_HEAD" != "$HASH" ]; then
echo "ERROR: Submodule HEAD commit does not match expected $HASH" >&2
exit 1
fi
echo "SUCCESS: Submodule commit $HASH verified."Length of output: 1028
Fix missing submodule commit on remote
The pinned commit eed25fc6e4f7e62c346a9d4ce5c7083e6500b7dd doesn’t exist on the submodule’s remote (https://github.com/meta-llama/llama-stack); push the intended commit or update the submodule reference to a valid hash.
🤖 Prompt for AI Agents
In llama-stack around lines 1 to 1, the repository is referencing a submodule
commit hash (eed25fc6e4f7e62c346a9d4ce5c7083e6500b7dd) that does not exist on
the submodule remote; either push the intended commit to the submodule remote or
update the superproject to point to a valid hash: enter the submodule directory,
run git fetch origin --tags, confirm the correct commit (or checkout the desired
branch and create/push the missing commit), then in the superproject update the
submodule to a valid commit (git add the submodule change and commit/push the
updated reference) so the submodule pointer on the remote matches an existing
commit.
scripts/build-images.sh
Outdated
|
|
||
| # Verify Red Hat subscription before building | ||
| check_redhat_subscription | ||
| # check_redhat_subscription |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Don’t silently drop the subscription precheck; gate it behind an opt-out flag.
Builds that rely on RHEL content will now fail later and less clearly. Make the check conditional on an env var or CLI flag instead of removing it.
Apply this diff to make the precheck skippable via SKIP_SUBSCRIPTION_CHECK=true:
-# check_redhat_subscription
+if [[ "${SKIP_SUBSCRIPTION_CHECK:-false}" != "true" ]]; then
+ check_redhat_subscription
+else
+ echo "Skipping Red Hat subscription check (SKIP_SUBSCRIPTION_CHECK=true)"
+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.
| # check_redhat_subscription | |
| if [[ "${SKIP_SUBSCRIPTION_CHECK:-false}" != "true" ]]; then | |
| check_redhat_subscription | |
| else | |
| echo "Skipping Red Hat subscription check (SKIP_SUBSCRIPTION_CHECK=true)" | |
| fi |
🤖 Prompt for AI Agents
In scripts/build-images.sh around line 145, the RHEL subscription precheck was
removed; restore the precheck but make it skippable by gating it on an
environment variable SKIP_SUBSCRIPTION_CHECK (default false). Reintroduce the
existing check invocation and wrap it in an if that only runs when
SKIP_SUBSCRIPTION_CHECK is not "true"; if the check fails, emit a clear error
and exit non-zero so builds that depend on RHEL content fail fast; when
SKIP_SUBSCRIPTION_CHECK="true" print a one-line notice that the check was
skipped.
the deployment time can only be tested in... deployment time 😅 |
|
/hold |
"local development" - |
|
Let's set some values in |
|
/lgtm |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 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 (2)
template-params.dev.env(1 hunks)template.yaml(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- template.yaml
⏰ 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 (2)
template-params.dev.env (2)
9-10: Good parameterization — sensible dev defaults.Moving hard-coded auth rules into deploy-time params is the right direction, and these dev values look aligned with the prior inlined config.
9-10: Correct Python invocation in verification script
Pass thetmplvariable into Python sosys.argv[1]isn’t empty. For example:#!/bin/bash set -euo pipefail tmpl=$(rg -nI --glob '**/*.{yaml,yml}' 'AUTH(Z|N)_ACCESS_RULES|AUTHN_ROLE_RULES' -l | head -n1) echo "Template: $tmpl" set -a # shellcheck disable=SC1091 source ./template-params.dev.env set +a python - "$tmpl" <<'PY' import os, re, sys, yaml from pathlib import Path tmpl_text = Path(sys.argv[1]).read_text() def repl(m): k = m.group(1) v = os.environ.get(k, '') if v == '': print(f"[WARN] Missing env for {k}", file=sys.stderr) return v rendered = re.sub(r"\$\{\{\s*([A-Z0-9_]+)\s*\}\}", repl, tmpl_text) # …rest of the checks… PYRerun this updated script and ensure no
[FAIL]appears, confirming the template renders correctly.
| AUTHN_ROLE_RULES='[{"jsonpath":"$.realm_access.roles[*]","operator":"contains","value":"redhat:employees","roles":["redhat_employee"]}]' | ||
| AUTHZ_ACCESS_RULES='[{"role":"redhat_employee","actions":["get_models","query","streaming_query","get_conversation","list_conversations","delete_conversation","feedback","info","get_metrics"]}]' |
There was a problem hiding this comment.
Fix the Pydantic validation: inject a YAML list, not a quoted string.
The reported error (authorization.access_rules.1.role=None / .actions missing) is consistent with the list being rendered incorrectly (e.g., as a quoted scalar or creating an extra empty item). Avoid wrapping the flow-style list in single quotes so it renders as a proper YAML list in the ConfigMap.
Apply this diff:
-AUTHN_ROLE_RULES='[{"jsonpath":"$.realm_access.roles[*]","operator":"contains","value":"redhat:employees","roles":["redhat_employee"]}]'
-AUTHZ_ACCESS_RULES='[{"role":"redhat_employee","actions":["get_models","query","streaming_query","get_conversation","list_conversations","delete_conversation","feedback","info","get_metrics"]}]'
+AUTHN_ROLE_RULES=[{"jsonpath":"$.realm_access.roles[*]","operator":"contains","value":"redhat:employees","roles":["redhat_employee"]}]
+AUTHZ_ACCESS_RULES=[{"role":"redhat_employee","actions":["get_models","query","streaming_query","get_conversation","list_conversations","delete_conversation","feedback","info","get_metrics"]}]Rationale:
- Unquoted flow-style JSON/YAML will be injected as a list node, not a string.
- Since the assignment is unquoted, keep values whitespace-free (already the case here) to avoid shell word-splitting when sourced locally.
📝 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.
| AUTHN_ROLE_RULES='[{"jsonpath":"$.realm_access.roles[*]","operator":"contains","value":"redhat:employees","roles":["redhat_employee"]}]' | |
| AUTHZ_ACCESS_RULES='[{"role":"redhat_employee","actions":["get_models","query","streaming_query","get_conversation","list_conversations","delete_conversation","feedback","info","get_metrics"]}]' | |
| # template-params.dev.env, lines 9–10 | |
| AUTHN_ROLE_RULES=[{"jsonpath":"$.realm_access.roles[*]","operator":"contains","value":"redhat:employees","roles":["redhat_employee"]}] | |
| AUTHZ_ACCESS_RULES=[{"role":"redhat_employee","actions":["get_models","query","streaming_query","get_conversation","list_conversations","delete_conversation","feedback","info","get_metrics"]}] |
🤖 Prompt for AI Agents
In template-params.dev.env around lines 9 to 10 the flow-style list values are
wrapped in single quotes, causing Pydantic to receive a string instead of a YAML
list and producing validation errors; remove the surrounding single quotes so
AUTHN_ROLE_RULES and AUTHZ_ACCESS_RULES are unquoted flow-style lists (keep them
whitespace-free) so they are injected as proper YAML list nodes into the
ConfigMap.
|
/unhold |
|
/hold let's wait for next week |
|
/unhold |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: carbonin, maorfr 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 |
openshift-merge-bot
bot
merged commit 16e8934
into
rh-ecosystem-edge:main
4 participants
follow up on:
this PR extracts the actual rules into parameters to be passed during deployment time via app-interface.
Summary by CodeRabbit