chore: update template & general improvement - #633
Conversation
|
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:
📝 WalkthroughWalkthroughThis PR updates the openrag-stack Helm chart to version 0.6.0 with shared helpers, configurable secrets, value-driven workloads, fullname-based resources, revised Ray and ingress behavior, deployment values, documentation, and nginx runtime permissions. ChangesOpenRAG Helm Chart Overhaul
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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
added
the
chore
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (1)
infra/charts/openrag-stack/values-linagora.yaml (1)
1-5: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winOverlay
storageClass: longhornwon't reach postgresql/milvus sub-charts.See companion comment on
values.yaml(persistence.storageClass anchor) — becausepostgresql.primary.persistence.storageClassandmilvus.minio/milvus.etcdstorageClass are set via a YAML alias to the base file'spersistence.storageClass, this overlay's override only updates the top-levelpersistence.storageClasskey and does not propagate to those sub-chart paths. Explicitly setpostgresql.primary.persistence.storageClass: longhorn,milvus.minio.persistence.storageClass: longhorn, andmilvus.etcd.persistence.storageClass: longhornhere as well.🤖 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 `@infra/charts/openrag-stack/values-linagora.yaml` around lines 1 - 5, The overlay only updates the top-level persistence.storageClass, so the PostgreSQL and Milvus sub-charts still keep the base-file alias values. Update this values overlay to explicitly set postgresql.primary.persistence.storageClass, milvus.minio.persistence.storageClass, and milvus.etcd.persistence.storageClass to longhorn alongside the existing persistence settings so the override applies everywhere.
🧹 Nitpick comments (1)
infra/charts/openrag-stack/values.yaml (1)
348-364: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
adminUi.ingressis missing the fields its own doc references.The comment above instructs setting the same
hoston both this Ingress block andopenrag.ingressfor single-vhost routing, butadminUi.ingressonly declaresenabled: true— noclassName,host,annotations, ortlsstubs likeopenrag.ingresshas (lines 411-423). If the admin-ui template reads these keys, they'll silently resolve to empty/nil; adding stub keys here would make the documented configuration surface discoverable.🤖 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 `@infra/charts/openrag-stack/values.yaml` around lines 348 - 364, The adminUi.ingress values block is missing the configuration keys referenced by its own routing note, so add stub fields for the admin UI ingress shape to match openrag.ingress. Update the ingress section in values.yaml to include the same discoverable keys (such as className, host, annotations, and tls) alongside enabled, so templates like admin-ui ingress rendering can resolve them consistently and the documented single-vhost setup is exposed in the chart values.
🤖 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 `@infra/charts/openrag-stack/templates/admin-ui.yaml`:
- Around line 25-27: The admin UI service account template is using a `with`
block that treats explicit `false` as empty, so `automountServiceAccountToken`
gets omitted from the manifest. Update the `adminUi` template logic to check for
nil/defined presence instead of truthiness so `false` is rendered intentionally,
and keep the field emission tied to the `automountServiceAccountToken` value in
`admin-ui.yaml`.
In `@infra/charts/openrag-stack/templates/infinity.yaml`:
- Around line 25-27: The `with` block around `automountServiceAccountToken` in
the infinity template skips rendering when the value is explicitly `false`, so
the setting is omitted instead of being enforced. Update the template logic to
check the presence of `.Values.reranker.automountServiceAccountToken` without
using `with`, and ensure the `automountServiceAccountToken` field is emitted for
both `true` and `false` values so the `reranker` service account configuration
is applied consistently.
In `@infra/charts/openrag-stack/templates/NOTES.txt`:
- Around line 14-18: Update the NOTES text to warn only about whole-list
overrides of vllm.servingEngineSpec.modelSpec, since Helm replaces entire list
values from a values file but an indexed --set on
modelSpec[N].hf_token.secretName only affects that single entry. In the
NOTES.txt guidance, keep the warning focused on replacing the full modelSpec
list and remove language that says an indexed --set wipes
repository/tag/modelURL/resources for all entries; reference the
vllm.servingEngineSpec.modelSpec and hf_token.secretName examples when revising
the message.
In `@infra/charts/openrag-stack/templates/openrag.yaml`:
- Around line 26-28: The automountServiceAccountToken template block in the
OpenRAG manifest is using a truthiness check, so a false value gets omitted
entirely. Update the conditional around the openrag.automountServiceAccountToken
field to check for key presence instead of value truthiness, so the OpenRAG
chart can render an explicit false and allow operators to disable service
account token automounting.
In `@infra/charts/openrag-stack/templates/raycluster.yaml`:
- Around line 159-190: The Ray dashboard Ingress in raycluster.yaml is exposing
an unauthenticated administrative surface; update the ingress template around
the ray.ingress block to prevent external access unless protection is explicitly
enforced. Require an internal-only ingress class or mandatory auth-related
annotations for the dashboard route, and make the ray.ingress values document
the risk and default to a safer, non-public configuration. Use the existing
ray.ingress.enabled, className, annotations, and backend service wiring to keep
the change localized.
- Around line 25-27: The conditional rendering around
automountServiceAccountToken in the Ray cluster template is using truthiness, so
a false value is omitted and Kubernetes falls back to the default. Update the
template logic in the head and worker specs to check for nil/defined value
instead of using with, so the automountServiceAccountToken field is emitted for
both true and false. Locate the affected blocks in raycluster.yaml around the
automountServiceAccountToken entries and apply the same fix in both places.
In `@infra/charts/openrag-stack/values.yaml`:
- Around line 90-100: The PostgreSQL password is currently anchored as a literal
in the postgresql.auth.password values block, which can leave
env.secrets.POSTGRES_PASSWORD out of sync when overrides are applied. Update the
chart wiring so postgresql.auth.password and env.secrets.POSTGRES_PASSWORD both
resolve from the same template-backed value, or enforce that they must be set
together; use the existing postgresql.auth.password and
env.secrets.POSTGRES_PASSWORD entries as the points to align.
---
Duplicate comments:
In `@infra/charts/openrag-stack/values-linagora.yaml`:
- Around line 1-5: The overlay only updates the top-level
persistence.storageClass, so the PostgreSQL and Milvus sub-charts still keep the
base-file alias values. Update this values overlay to explicitly set
postgresql.primary.persistence.storageClass,
milvus.minio.persistence.storageClass, and milvus.etcd.persistence.storageClass
to longhorn alongside the existing persistence settings so the override applies
everywhere.
---
Nitpick comments:
In `@infra/charts/openrag-stack/values.yaml`:
- Around line 348-364: The adminUi.ingress values block is missing the
configuration keys referenced by its own routing note, so add stub fields for
the admin UI ingress shape to match openrag.ingress. Update the ingress section
in values.yaml to include the same discoverable keys (such as className, host,
annotations, and tls) alongside enabled, so templates like admin-ui ingress
rendering can resolve them consistently and the documented single-vhost setup is
exposed in the chart values.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c0a0788f-d49f-4518-a9c6-dabab8c94513
⛔ Files ignored due to path filters (1)
infra/charts/openrag-stack/Chart.lockis excluded by!**/*.lock
📒 Files selected for processing (18)
.github/workflows/build_rc.ymldocs/content/docs/documentation/kubernetes.mdinfra/charts/openrag-stack/Chart.yamlinfra/charts/openrag-stack/templates/NOTES.txtinfra/charts/openrag-stack/templates/_helpers.tplinfra/charts/openrag-stack/templates/admin-ui.yamlinfra/charts/openrag-stack/templates/configmap-env.yamlinfra/charts/openrag-stack/templates/extra-objects.yamlinfra/charts/openrag-stack/templates/infinity.yamlinfra/charts/openrag-stack/templates/ingress.yamlinfra/charts/openrag-stack/templates/openrag.yamlinfra/charts/openrag-stack/templates/postgres-migration-job.yamlinfra/charts/openrag-stack/templates/pvc.yamlinfra/charts/openrag-stack/templates/raycluster.yamlinfra/charts/openrag-stack/templates/secrets-env.yamlinfra/charts/openrag-stack/values-linagora.yamlinfra/charts/openrag-stack/values.yamlinfra/docker/ui.Dockerfile
💤 Files with no reviewable changes (1)
- infra/charts/openrag-stack/templates/ingress.yaml
Ahmath-Gadji
left a comment
Ahmath-Gadji
left a comment
There was a problem hiding this comment.
Nice modernization overall — the NOTES.txt list-merge warning, proxy fail-fast checks, checksum/config annotation and the pinned infinity tag are all welcome. I rendered the chart with helm template and left inline comments on a handful of issues that I think should be addressed before merge (a few are confirmed rendering bugs, a few are security regressions vs the previous chart).
Two concerns that don't fit a single line:
Upgrade path / breaking renames. Every resource is renamed (rag-* → openrag-*, Release.Name-* → fullname-based), and postgresql/milvus gain fullnameOverrides. For any existing release not named openrag, an upgrade renames the Postgres/Milvus StatefulSets and creates fresh empty PVCs; even for a release named openrag, the app PVCs change name (rag-data → openrag-data), orphaning existing data (the old PVCs only survive via helm.sh/resource-policy: keep). The selector-label change (app.kubernetes.io/instance: .Release.Name → fullname) is part of the same story. Could we get an upgrade/migration note in NOTES.txt or docs/content/docs/documentation/kubernetes.md spelling this out?
venv coordination removed. The init-venv-sync / init-venv init containers (uv sync + .ready flag on the shared RWX venv PVC) are gone from both the openrag Deployment and the RayCluster, but the venv PVC is still mounted at /app/.venv everywhere. What populates the venv now, and what prevents the head, workers and app pod from racing concurrent uv runs on the same PVC at first boot? If the v2.0.0-rc.1 images bake the venv into the image, the PVC mount (which shadows it) should probably go too.
andyne13
left a comment
andyne13
left a comment
There was a problem hiding this comment.
Thanks @ThibautChoppy. I resolved the Chart.yaml conflict with develop (kept your version: 0.6.0 and the added metadata; set appVersion: "2.0.0" for current GA), which triggered CI. Everything is green except tests (3.12), which fails 5 Helm security-hardening checks — this branch predates that hardening on develop, so the chart currently regresses it. Please reconcile with develop:
- Placeholder DB secret —
postgresql.auth.passwordmust not default toCHANGE_ME_STRONG_PASSWORD(empty / secret ref). - Ray dashboard —
ray.dashboardHostmust default to127.0.0.1, not0.0.0.0(ShadowRay / CVE-2023-48022). - securityContexts — restore the restricted
securityContextblocks on the workloads. - Ingress —
templates/ingress.yamlwas removed; keep the hardened template (not exposed by default + TLS). - Migration Job — reference the
rag-envsecret instead of inlining literal secret values.
The two test files are the exact spec: tests/unit/infra/test_helm_security_hardening.py and tests/unit/infra/test_postgres_migration_job_template.py. uv run pytest tests/unit/infra/ -q should be green before pushing (that re-runs CI). The rest of the PR looks good — and if it's easy, splitting the app-code commits from the Helm work would review faster, though not required.
Every other optional model-registry toggle is explicitly listed in the chart's env.config for discoverability (RERANKER_ENABLED, WITH_CHAINLIT_UI, ...). This one was missing entirely, so Helm-based deployments had no discoverable way to opt in even though the generic config map passthrough already supported it.
…set warning - values-linagora.yaml: persistence.storageClass only reaches the chart's own 4 PVCs — the postgresql/milvus/minio/etcd sub-charts read it through a YAML alias resolved before Helm merges this overlay, so it never reached them. Set storageClass explicitly per sub-chart here too. Verified with `helm template`: postgresql/etcd/minio now render storageClassName: longhorn alongside the 4 chart PVCs. - NOTES.txt: the HF_TOKEN mismatch warning wrongly implied an indexed --set (modelSpec[N].hf_token.secretName=...) wipes sibling entries. Helm only replaces whole lists across values sources — an indexed override is safe. Narrowed the warning to the actual risk (redefining the entire modelSpec array). Addresses the two open review threads on PR linagora#633.
|
LGTM, waiting for the next release version for image tag update in valuefile before merge |
…ement # Conflicts: # infra/charts/openrag-stack/Chart.yaml # infra/charts/openrag-stack/values.yaml
andyne13
left a comment
andyne13
left a comment
There was a problem hiding this comment.
Thanks @ThibautChoppy — I re-checked the five points from my earlier review and they're all addressed: the DB password placeholder is gone, ray.dashboardHost defaults to loopback, the shared security block is applied through the merge helpers, the ingress is off by default behind a required host guard, and the migration Job now goes through envFrom with no inlined literals. tests/unit/infra/ is green at this head. The secrets-env.yaml work is really nice — reading POSTGRES_PASSWORD live from postgresql.auth.password instead of the YAML anchor is a subtle trap well avoided, and the presence-based merge helpers neatly dodge Sprig merge's zero-value behaviour.
One thing I ran into while testing the built image, and I think it just needs a one-liner in infra/docker/ui.Dockerfile:
The new chown -R 10001:10001 lines up with the chart's podSecurityContext, but the image still ends with USER nginx, which is UID 101 in nginxinc/nginx-unprivileged. Under Helm that's all fine since the pod forces runAsUser: 10001 — it's only docker compose, where nothing overrides the user, that ends up running as 101 against dirs owned by 10001. Building from this branch:
$ docker exec <admin-ui> sh -c 'id; ls -ldn /var/cache/nginx; touch /var/cache/nginx/probe'
uid=101(nginx) gid=101(nginx)
drwxrwxr-x 1 10001 10001 /var/cache/nginx
touch: /var/cache/nginx/probe: Permission denied
Took me a while to spot, because the container starts and serves the UI perfectly happily — it only shows up once nginx spools a request body to client_temp, which is what client_max_body_size 1024M in the admin conf is there for. Either USER 10001:10001 in the Dockerfile or user: "10001:10001" on the compose service should cover it, whichever you prefer.
Non-blocking, just worth a line in the docs while you're here: the PVCs/ConfigMap/Secret get renamed (rag-data → {{ fullname }}-data, etc.). helm.sh/resource-policy: keep means existing volumes survive, but they're no longer mounted, so an upgraded release would come up with empty volumes unless the operator sets fullnameOverride or migrates. An upgrade note would save someone a confusing afternoon.
Two small things for later, no need to touch them here: mergeSecurityContext is a shallow merge, so a component overriding capabilities would drop the shared drop: [ALL] (nothing does today); and the three vllm modelSpec entries still default to tag: "latest" — your own note says to pin, so the defaults could follow suit.
Happy to approve once the UID bit is sorted.
|
@andyne13 — following up on your 2026-07-16 review. All 5 issues you flagged look resolved as of the current head (15db9a1):
The branch is also conflict-free against Could you take another look and update/clear your review when you get a chance? Thanks for catching these the first time around. |
values.yaml described admin-ui and openrag/ray-serve as two separate Ingress resources needing a shared host, but the chart now renders a single combined Ingress in openrag.yaml where adminUi.ingress.enabled only toggles the /app/ path. adminUi.ingress has no host/className/tls fields at all, so the old text could lead an operator to configure fields the template silently ignores.
|
@Ahmath-Gadji thanks for the follow-up — agreed, and I confirmed the same five myself in this comment last night (I also re-ran That comment is easy to miss though, because it also raised a sixth item that came out of building the image, and it's still open at
For reference, the base image ships that directory owned by 101, so the chown is what introduces it: It's easy to miss because the container still starts and serves the UI normally (I got a 301 from Either Also non-blocking, from the same pass: the PVCs/ConfigMap/Secret are renamed ( Happy to approve as soon as the UID bit is sorted. |
|
@ThibautChoppy one last thing before I can approve — a one-liner in Short version: the new Either It's an easy one to miss because the container still starts and serves the UI normally; it only bites when nginx spools a request body to Everything else from my 16 July review is confirmed resolved (thanks @Ahmath-Gadji for the follow-up), and |
|
@ThibautChoppy thanks for the work here — the five points from my 16 July review are all resolved, and Correcting something from my earlier comment, where I framed this as compose-only: 1. Under docker compose this is broken — nothing overrides the user, so the container runs as 101 against directories owned by 10001. Built from this branch: The base image ships that directory owned by 101, so the 2. On OpenShift it depends on your SCC — and here I'd rather ask than assert, since you know the target better than I do. Under the default The fix that covers both, aligning -RUN chown -R 10001:10001 /var/cache/nginx /etc/nginx/conf.d /var/run && \
+RUN chown -R 10001:0 /var/cache/nginx /etc/nginx/conf.d /var/run && \
chmod -R g+w /var/cache/nginx /etc/nginx/conf.d /var/run
-USER nginx
+USER 10001:0plus Worth saying why this is easy to miss: the container starts and serves the UI normally, so neither CI nor a smoke test catches it. It only fails when nginx spools a request body to Non-blocking, for the docs whenever you touch them: the PVCs/ConfigMap/Secret are renamed ( Let me know about the SCC, and I'll approve as soon as the UID part is sorted. |
infra/docker/ui.Dockerfile chowned nginx's runtime paths to 10001:10001 but
left USER as the base image's `nginx` (uid 101, gid 101 only), so the running
user had neither owner nor group access to them. The Helm chart matched the
chown with adminUi.podSecurityContext.runAsGroup: 10001, which only works on a
cluster whose SCC allows pinning that GID.
Switch to the same arbitrary-UID pattern api.Dockerfile already uses: chown to
group 0, chmod g+w, and USER 10001:0, with adminUi.podSecurityContext.runAsGroup
set to 0 to match. Under compose the user then owns the paths; under OpenShift's
restricted-v2 SCC an arbitrary UID writes through group 0. The base image itself
ships these paths as 101:0 for the same reason.
Nothing writes under /var/cache/nginx today — nginx-unprivileged points every
*_temp_path at /tmp and openrag-admin.conf sets `proxy_cache off` — so the
mismatch was latent rather than actively breaking uploads. The one observable
symptom was docker-entrypoint.d/10-listen-on-ipv6-by-default.sh failing to
patch /etc/nginx/conf.d/default.conf at startup.
Add regression tests pinning the image's USER/ownership and the chart's
runAsGroup, and document the 0.6.0 rag-* -> {{ fullname }}-* resource rename,
which leaves kept PVCs unmounted unless the operator overrides fullnameOverride
or migrates the data.
templates/postgres-migration-job.yaml runs the OpenRAG image with the same
pinned securityContext as its Deployment (runAsUser: 10001, runAsGroup: 0) but
never rendered a serviceAccountName, so it fell back to the namespace's
`default` ServiceAccount.
On OpenShift that puts the Job under restricted-v2 (MustRunAsRange) rather than
whatever SecurityContextConstraints is bound to the app's own ServiceAccount,
and admission rejects a runAsUser outside the namespace's assigned UID range.
Because the Job is a pre-install/pre-upgrade hook, that failure aborts the whole
release — and it only shows up once postgresProvisioning.migrationJob is enabled,
which is exactly the managed-Postgres setup the docs recommend on OpenShift.
Reuse openrag.serviceAccountName through tpl, like the Deployment does, so a
templated value such as "{{ .Release.Name }}-openrag" resolves the same way.
andyne13
left a comment
andyne13
left a comment
There was a problem hiding this comment.
Approving.
The admin-ui permission blocker is resolved — re-verified empirically by replicating the final stage on the base image and probing /var/cache/nginx, /etc/nginx/conf.d and /var/run:
| variant | runs as | writable |
|---|---|---|
before (chown 10001:10001 + USER nginx) |
uid=101 gid=101 | denied on all three |
after (chown 10001:0 + USER 10001:0) |
uid=10001 gid=0 | OK on all three |
| after, arbitrary UID (restricted-v2 sim) | uid=1000670000 gid=0 | OK on all three |
Taking the portable variant over the minimal one-line fix was the right call: it repairs compose and makes the chart work under a restricted SCC, not only a RunAsAny one — which matters for a chart that ships publicly.
fix(helm): give the migration Job the same ServiceAccount as the app — correct. It renders the SA exactly as templates/openrag.yaml does, so a templated value resolves identically in both and an unset value behaves as before. The rationale holds: the Job pins runAsUser: 10001, which under the namespace's default SA lands on restricted-v2 and gets rejected — and as a pre-upgrade hook that aborts the whole release. Test covers it.
Six checks green on 70f3ab8e, no unresolved threads.
Merge note: Chart.yaml moves to version: 0.6.0 and keeps appVersion: "2.0.1" — appVersion and the image tags are set by the version bump, so nothing to change here.
Follow-up, not blocking: you flagged that raycluster and infinity weren't covered by the security-context changes. Worth a separate issue so it doesn't get lost — happy to file it.
|
Thanks Thibaut for the work here, and for the migration Job ServiceAccount fix. Everything is green on my side — approved, and we're merging this for 2.1.0. |
3 participants
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests