chore: update template & general improvement#456
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:
📝 WalkthroughWalkthroughUpgrades chart to v0.5.0: adds a secret-name helper and multi-provider secret rendering, makes resource names release-scoped, introduces pod/container securityContexts and health probes, tightens dependency ranges, and updates values for Ray, vLLM multi-models, ingress, and reranker wiring. ChangesOpenRAG Stack Helm Release Update
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
charts/openrag-stack/templates/openrag.yaml (1)
42-46: ⚖️ Poor tradeoffConsider enabling
readOnlyRootFilesystemin the future for additional hardening.The container security context sets
readOnlyRootFilesystem: false, which may be necessary for the application to function. For additional security hardening in the future, consider whether the application could work with a read-only root filesystem and useemptyDirtmpfs volumes for writable paths.🤖 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 `@charts/openrag-stack/templates/openrag.yaml` around lines 42 - 46, The securityContext currently sets readOnlyRootFilesystem: false; evaluate whether the application components (pods defined in this chart) can operate with readOnlyRootFilesystem: true and, if so, switch readOnlyRootFilesystem to true for hardened defaults and add explicit writable tmpfs emptyDir volumes for any runtime-write paths; update the pod templates where securityContext is defined (look for securityContext and readOnlyRootFilesystem keys in the chart) to set readOnlyRootFilesystem: true and add matching emptyDir mounts (medium: "Memory") for directories the app must write to, validating startup and runtime behavior before committing the change.charts/openrag-stack/templates/_helpers.tpl (1)
64-70: 💤 Low valueConsider translating the comment to English for consistency.
The comment on lines 65-66 is in French while all other comments in this file are in English. For maintainability and consistency, consider translating it.
📝 Proposed translation
{{/* -Nom du Secret d'environnement, utilisé par tous les consommateurs. -Si env.existingSecret est renseigné, c'est lui qui est utilisé. +Environment Secret name, used by all consumers. +If env.existingSecret is set, it takes precedence. */}} {{- define "openrag-stack.secretName" -}} {{- .Values.env.existingSecret | default (printf "%s-env-secrets" .Release.Name) }} {{- end }}🤖 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 `@charts/openrag-stack/templates/_helpers.tpl` around lines 64 - 70, Translate the French comment above the Helm helper into English for consistency: replace the French lines describing the environment Secret with an English equivalent (e.g., "Name of the environment Secret used by consumers. If .Values.env.existingSecret is set, it is used.") near the define "openrag-stack.secretName" so the comment matches the rest of the file.charts/openrag-stack/templates/secrets-env.yaml (2)
40-40: 💤 Low valueTranslate error message to English for consistency.
The error message is in French. For codebase consistency, consider translating to English.
📝 Proposed translation
{{- else }} - {{- fail "externalSecret requiert dataFrom ou data" }} + {{- fail "externalSecret requires either dataFrom or data" }} {{- end }}🤖 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 `@charts/openrag-stack/templates/secrets-env.yaml` at line 40, The error message in the Helm template uses French: the fail call in the template (the string passed to fail in templates/secrets-env.yaml) should be translated to English for consistency; update the fail message text (the literal inside the {{- fail "..." }}) to an English phrase such as "externalSecret requires dataFrom or data" so the fail invocation message is fully in English while leaving the fail call and template logic unchanged.
72-74: 💤 Low valueTranslate error message to English for consistency.
The error message is in French. For codebase consistency, consider translating to English.
📝 Proposed translation
{{- else }} -{{- fail (printf "env.secretsProvider.type invalide : %q (valeurs acceptées : values, externalSecret, vaultStaticSecret)" $type) }} +{{- fail (printf "Invalid env.secretsProvider.type: %q (accepted values: values, externalSecret, vaultStaticSecret)" $type) }} {{- end }}🤖 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 `@charts/openrag-stack/templates/secrets-env.yaml` around lines 72 - 74, The fail call currently raises a French error message; update the printf string so the fail(...) uses an English message and retains the $type interpolation and accepted values list — e.g. replace "env.secretsProvider.type invalide : %q (valeurs acceptées : values, externalSecret, vaultStaticSecret)" with an English equivalent like "invalid env.secretsProvider.type: %q (accepted values: values, externalSecret, vaultStaticSecret)" so the template's fail(...) and $type variable usage remain unchanged.charts/openrag-stack/values.yaml (2)
274-307: 💤 Low valueConsider translating French comments to English for consistency.
The comments in lines 274-276, 278-280, 283, 289-298, and 300 are in French. For codebase consistency, consider translating them to English.
🤖 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 `@charts/openrag-stack/values.yaml` around lines 274 - 307, Update the in-file comments in values.yaml so they are all English for consistency: translate the French comment blocks around existingSecret, the "Secrets provider" section, and the externalSecret and vaultStaticSecret option descriptions (including lines describing dataFrom/data and vaultStaticSecret fields like vaultAuthRef, mount, type, path, refreshAfter, hmacSecretData). Keep the same intent and wording clarity, preserving original keys and YAML structure (existingSecret, secretsProvider.type, externalSecret, dataFrom, data, vaultStaticSecret) while replacing only the comment text.
212-212: 💤 Low valueConsider translating comment to English for consistency.
The inline comment is in French. For consistency with the rest of the codebase, consider translating to English:
# Set to "" to disable.🤖 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 `@charts/openrag-stack/values.yaml` at line 212, The inline comment on the runtimeClassName setting (runtimeClassName: nvidia) is in French; update it to English to match the codebase (e.g., change the comment to "# Set to \"\" to disable") while leaving the key and value 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.
Inline comments:
In `@charts/openrag-stack/templates/ingress.yaml`:
- Around line 17-23: The ingress regex /api(/|$)(.*) causes the first capture
group to be the delimiter, breaking the rewrite
nginx.ingress.kubernetes.io/rewrite-target: /$1; change the path to use a
non-capturing delimiter so the suffix becomes capture group 1 (e.g.
/api(?:/|$)(.*)) and keep the rewrite-target as /$1 to correctly map the API
suffix to the backend; update the path value where it is defined in the ingress
template.
In `@charts/openrag-stack/values.schema.json`:
- Line 4: The schema currently marks "persistence" as required which forces
persistence.storageClass even when persistence is intentionally disabled; update
charts/openrag-stack/values.schema.json so persistence is optional (remove
"persistence" from the top-level "required" array) and instead make
persistence.storageClass conditional by not requiring it unconditionally—ensure
the persistence block can be omitted when .Values.persistence.enabled is false
to match templates/pvc.yaml's {{- if .Values.persistence.enabled }} guard; keep
"openrag" required since templates/openrag.yaml and templates/ingress.yaml
access .Values.openrag.* without an .enabled gate.
---
Nitpick comments:
In `@charts/openrag-stack/templates/_helpers.tpl`:
- Around line 64-70: Translate the French comment above the Helm helper into
English for consistency: replace the French lines describing the environment
Secret with an English equivalent (e.g., "Name of the environment Secret used by
consumers. If .Values.env.existingSecret is set, it is used.") near the define
"openrag-stack.secretName" so the comment matches the rest of the file.
In `@charts/openrag-stack/templates/openrag.yaml`:
- Around line 42-46: The securityContext currently sets readOnlyRootFilesystem:
false; evaluate whether the application components (pods defined in this chart)
can operate with readOnlyRootFilesystem: true and, if so, switch
readOnlyRootFilesystem to true for hardened defaults and add explicit writable
tmpfs emptyDir volumes for any runtime-write paths; update the pod templates
where securityContext is defined (look for securityContext and
readOnlyRootFilesystem keys in the chart) to set readOnlyRootFilesystem: true
and add matching emptyDir mounts (medium: "Memory") for directories the app must
write to, validating startup and runtime behavior before committing the change.
In `@charts/openrag-stack/templates/secrets-env.yaml`:
- Line 40: The error message in the Helm template uses French: the fail call in
the template (the string passed to fail in templates/secrets-env.yaml) should be
translated to English for consistency; update the fail message text (the literal
inside the {{- fail "..." }}) to an English phrase such as "externalSecret
requires dataFrom or data" so the fail invocation message is fully in English
while leaving the fail call and template logic unchanged.
- Around line 72-74: The fail call currently raises a French error message;
update the printf string so the fail(...) uses an English message and retains
the $type interpolation and accepted values list — e.g. replace
"env.secretsProvider.type invalide : %q (valeurs acceptées : values,
externalSecret, vaultStaticSecret)" with an English equivalent like "invalid
env.secretsProvider.type: %q (accepted values: values, externalSecret,
vaultStaticSecret)" so the template's fail(...) and $type variable usage remain
unchanged.
In `@charts/openrag-stack/values.yaml`:
- Around line 274-307: Update the in-file comments in values.yaml so they are
all English for consistency: translate the French comment blocks around
existingSecret, the "Secrets provider" section, and the externalSecret and
vaultStaticSecret option descriptions (including lines describing dataFrom/data
and vaultStaticSecret fields like vaultAuthRef, mount, type, path, refreshAfter,
hmacSecretData). Keep the same intent and wording clarity, preserving original
keys and YAML structure (existingSecret, secretsProvider.type, externalSecret,
dataFrom, data, vaultStaticSecret) while replacing only the comment text.
- Line 212: The inline comment on the runtimeClassName setting
(runtimeClassName: nvidia) is in French; update it to English to match the
codebase (e.g., change the comment to "# Set to \"\" to disable") while leaving
the key and value unchanged.
🪄 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: b5bc4bd1-ea05-42a4-b969-7577d74f118f
⛔ Files ignored due to path filters (1)
charts/openrag-stack/Chart.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
charts/openrag-stack/Chart.yamlcharts/openrag-stack/templates/_helpers.tplcharts/openrag-stack/templates/indexer-ui.yamlcharts/openrag-stack/templates/infinity.yamlcharts/openrag-stack/templates/ingress.yamlcharts/openrag-stack/templates/openrag.yamlcharts/openrag-stack/templates/pvc.yamlcharts/openrag-stack/templates/raycluster.yamlcharts/openrag-stack/templates/secrets-env.yamlcharts/openrag-stack/values.schema.jsoncharts/openrag-stack/values.yaml
| { | ||
| "$schema": "http://json-schema.org/draft-07/schema#", | ||
| "type": "object", | ||
| "required": ["persistence", "openrag"], |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify persistence and openrag usage patterns in templates
# Check if persistence is conditionally rendered
echo "=== Checking persistence conditional usage ==="
rg -n "\.Values\.persistence\.enabled" charts/openrag-stack/templates/
# Check if openrag can be disabled
echo -e "\n=== Checking if openrag can be disabled ==="
rg -n "\.Values\.openrag\.enabled" charts/openrag-stack/templates/
# Check default values
echo -e "\n=== Checking default values ==="
rg -n "^persistence:" charts/openrag-stack/values.yaml -A 2
rg -n "^openrag:" charts/openrag-stack/values.yaml -A 2Repository: linagora/openrag
Length of output: 404
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Find any openrag conditional rendering in templates ==="
rg -n "if\s+.*\.Values\.openrag|\.Values\.openrag\." charts/openrag-stack/templates/
echo -e "\n=== Check values.yaml for openrag.enabled / enabled under openrag ==="
rg -n "^\s*openrag:|^\s*enabled:" charts/openrag-stack/values.yaml -n -C 2
echo -e "\n=== Inspect values.schema.json required blocks for openrag/persistence ==="
rg -n "\"required\"\s*:\s*\[\"persistence\"\s*,\s*\"openrag\"|\"required\"\s*:" charts/openrag-stack/values.schema.json -n -C 2
echo -e "\n=== Find openrag properties (and any enabled) in values.schema.json ==="
rg -n "\"openrag\"|\"enabled\"" charts/openrag-stack/values.schema.json -n -C 2Repository: linagora/openrag
Length of output: 3720
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="charts/openrag-stack/values.schema.json"
echo "=== Extract persistence block (around required/properties/enabled) ==="
python3 - <<'PY'
import json,sys
path="charts/openrag-stack/values.schema.json"
with open(path) as f:
data=json.load(f)
p=data.get("properties",{}).get("persistence",{})
print("persistence keys:", sorted(p.keys()))
print("persistence.required:", p.get("required"))
print("persistence.properties keys:", sorted((p.get("properties") or {}).keys()))
print("persistence.properties.enabled:", (p.get("properties") or {}).get("enabled"))
PY
echo -e "\n=== Extract openrag required/properties (and whether enabled exists) ==="
python3 - <<'PY'
import json,sys
path="charts/openrag-stack/values.schema.json"
with open(path) as f:
data=json.load(f)
o=data.get("properties",{}).get("openrag",{})
print("openrag keys:", sorted(o.keys()))
print("openrag.required:", o.get("required"))
print("openrag.properties keys:", sorted((o.get("properties") or {}).keys()))
print("openrag.properties.enabled:", (o.get("properties") or {}).get("enabled"))
PYRepository: linagora/openrag
Length of output: 511
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "\.Values\.persistence\.storageClass" charts/openrag-stack/templates/Repository: linagora/openrag
Length of output: 570
Fix schema “required” for persistence to match conditional template rendering
charts/openrag-stack/templates/pvc.yamlonly renders PVCs when{{- if .Values.persistence.enabled }}, butcharts/openrag-stack/values.schema.jsonrequirespersistence.storageClassunconditionally (persistence.required: ["storageClass"]). This can reject valid values wherepersistence.enabled=false(andstorageClassis intentionally omitted).openraghas no.enabledgate in templates (e.g.,templates/openrag.yamlandtemplates/ingress.yamlreference.Values.openrag.*directly), so making top-levelopenragrequired is consistent.
🤖 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 `@charts/openrag-stack/values.schema.json` at line 4, The schema currently
marks "persistence" as required which forces persistence.storageClass even when
persistence is intentionally disabled; update
charts/openrag-stack/values.schema.json so persistence is optional (remove
"persistence" from the top-level "required" array) and instead make
persistence.storageClass conditional by not requiring it unconditionally—ensure
the persistence block can be omitted when .Values.persistence.enabled is false
to match templates/pvc.yaml's {{- if .Values.persistence.enabled }} guard; keep
"openrag" required since templates/openrag.yaml and templates/ingress.yaml
access .Values.openrag.* without an .enabled gate.
|
+1 |
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 `@charts/openrag-stack/values.yaml`:
- Around line 90-97: The values.yaml currently hardcodes hfTokenSecretName:
&vllmHfTokenSecretName "rag-env-secrets", which couples secret lookup to a
release named "rag" and breaks deployments with other release names; change the
chart to accept a configurable top-level value and propagate it to the vLLM
subchart instead of a literal: introduce a chart-level value (hfTokenSecretName)
used by vllm.servingEngineSpec.modelSpec[*].hf_token.secretName (or reference
via global values) so users can override once (or inherit from .Release.Name by
default), and update the README to document overriding hfTokenSecretName when
the release name differs from "rag".
🪄 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: cec35ef7-083a-42b7-a785-33227aaef471
📒 Files selected for processing (6)
charts/openrag-stack/templates/NOTES.txtcharts/openrag-stack/templates/_helpers.tplcharts/openrag-stack/templates/indexer-ui.yamlcharts/openrag-stack/templates/infinity.yamlcharts/openrag-stack/templates/openrag.yamlcharts/openrag-stack/values.yaml
🚧 Files skipped from review as they are similar to previous changes (5)
- charts/openrag-stack/templates/_helpers.tpl
- charts/openrag-stack/templates/NOTES.txt
- charts/openrag-stack/templates/indexer-ui.yaml
- charts/openrag-stack/templates/infinity.yaml
- charts/openrag-stack/templates/openrag.yaml
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 `@charts/openrag-stack/templates/configmap-env.yaml`:
- Around line 9-16: The template sets proxy env vars when .Values.proxy.enabled
is true but doesn't validate .Values.proxy.url; update
charts/openrag-stack/templates/configmap-env.yaml to add a conditional check
that .Values.proxy.url is non-empty and not the placeholder before emitting
HTTP_PROXY/HTTPS_PROXY/http_proxy/https_proxy, and similarly ensure
.Values.proxy.noProxy is validated before NO_PROXY/no_proxy is emitted; if the
URL is missing or placeholder, fail Helm rendering with a clear error (use
helm's required or fail functions) or skip injecting the proxy vars and
log/describe the validation failure.
🪄 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: e1c77bd9-d2eb-4d8c-81b7-fc165de6b6a9
📒 Files selected for processing (3)
charts/openrag-stack/templates/configmap-env.yamlcharts/openrag-stack/templates/openrag.yamlcharts/openrag-stack/values.yaml
🚧 Files skipped from review as they are similar to previous changes (2)
- charts/openrag-stack/templates/openrag.yaml
- charts/openrag-stack/values.yaml
| {{- if .Values.proxy.enabled }} | ||
| HTTP_PROXY: {{ .Values.proxy.url | quote }} | ||
| HTTPS_PROXY: {{ .Values.proxy.url | quote }} | ||
| http_proxy: {{ .Values.proxy.url | quote }} | ||
| https_proxy: {{ .Values.proxy.url | quote }} | ||
| NO_PROXY: {{ .Values.proxy.noProxy | quote }} | ||
| no_proxy: {{ .Values.proxy.noProxy | quote }} | ||
| {{- end }} |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add validation that proxy.url is set when proxy is enabled.
The template injects proxy environment variables when .Values.proxy.enabled is true, but doesn't validate that .Values.proxy.url contains a valid value. If a user enables the proxy but forgets to customize the URL from the placeholder default (http://192.168.100.100:80) or sets it to an empty string, all workloads consuming this ConfigMap will have broken proxy configuration, preventing external connectivity.
🛡️ Add a validation check
{{- if .Values.proxy.enabled }}
+{{- if not .Values.proxy.url }}
+{{- fail "proxy.url must be set when proxy.enabled is true" }}
+{{- end }}
HTTP_PROXY: {{ .Values.proxy.url | quote }}
HTTPS_PROXY: {{ .Values.proxy.url | quote }}📝 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 .Values.proxy.enabled }} | |
| HTTP_PROXY: {{ .Values.proxy.url | quote }} | |
| HTTPS_PROXY: {{ .Values.proxy.url | quote }} | |
| http_proxy: {{ .Values.proxy.url | quote }} | |
| https_proxy: {{ .Values.proxy.url | quote }} | |
| NO_PROXY: {{ .Values.proxy.noProxy | quote }} | |
| no_proxy: {{ .Values.proxy.noProxy | quote }} | |
| {{- end }} | |
| {{- if .Values.proxy.enabled }} | |
| {{- if not .Values.proxy.url }} | |
| {{- fail "proxy.url must be set when proxy.enabled is true" }} | |
| {{- end }} | |
| HTTP_PROXY: {{ .Values.proxy.url | quote }} | |
| HTTPS_PROXY: {{ .Values.proxy.url | quote }} | |
| http_proxy: {{ .Values.proxy.url | quote }} | |
| https_proxy: {{ .Values.proxy.url | quote }} | |
| NO_PROXY: {{ .Values.proxy.noProxy | quote }} | |
| no_proxy: {{ .Values.proxy.noProxy | quote }} | |
| {{- end }} |
🤖 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 `@charts/openrag-stack/templates/configmap-env.yaml` around lines 9 - 16, The
template sets proxy env vars when .Values.proxy.enabled is true but doesn't
validate .Values.proxy.url; update
charts/openrag-stack/templates/configmap-env.yaml to add a conditional check
that .Values.proxy.url is non-empty and not the placeholder before emitting
HTTP_PROXY/HTTPS_PROXY/http_proxy/https_proxy, and similarly ensure
.Values.proxy.noProxy is validated before NO_PROXY/no_proxy is emitted; if the
URL is missing or placeholder, fail Helm rendering with a clear error (use
helm's required or fail functions) or skip injecting the proxy vars and
log/describe the validation failure.
Sync with current stream
- Configurable probes (exec ou httpGet) via values, avec support du mode Ray Serve - Support TLS et annotations configurables dans l'ingress - SecurityContext pour openrag, reranker et indexer-ui - Proxy HTTP sortant optionnel - Secrets provider configurable (values, ExternalSecret, VaultStaticSecret) - Réranker optionnel avec URL externe - Champs kuberay.enabled et ray.enabled pour désactiver les composants - Paramètres vllm et reranker externalisés hors des ancres YAML - Corrections indentation, pathType ingress, SC raycluster/indexer - Ajout NOTES.txt, extra-objects.yaml, networkpolicy.yaml - values-linagora.yaml pour les overrides spécifiques Linagora Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
ThibautChoppy
force-pushed
the
feat/helm_improvement
branch
from
2cc9113 to
8db61c9
Compare
- Configurable probes (exec ou httpGet) via values, avec support du mode Ray Serve - Support TLS et annotations configurables dans l'ingress - SecurityContext pour openrag, reranker et indexer-ui - Proxy HTTP sortant optionnel - Secrets provider configurable (values, ExternalSecret, VaultStaticSecret) - Réranker optionnel avec URL externe - Champs kuberay.enabled et ray.enabled pour désactiver les composants - Paramètres vllm et reranker externalisés hors des ancres YAML - Corrections indentation, pathType ingress, SC raycluster/indexer - Ajout NOTES.txt, extra-objects.yaml, networkpolicy.yaml - values-linagora.yaml pour les overrides spécifiques Linagora Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…rag into feat/helm_improvement
Sync with current stream
Sync with current stream
| {{- if .Values.ingress.paths.openrag.enabled }} | ||
| - path: /(.*) | ||
| pathType: ImplementationSpecific | ||
| pathType: Prefix |
There was a problem hiding this comment.
Ingress does not route anything with default values.
With the default values there are no ingress annotations and pathType is set to Prefix, but the path is /(.*). Prefix matching does not understand regex, so it looks for a request path that literally starts with /(.*). Normal requests like /v1/chat/completions will not match and traffic goes nowhere. It only works once you add the nginx use-regex and rewrite-target annotations, and right now only values-linagora.yaml has those. Either set pathType: ImplementationSpecific back for this regex path, or put the working nginx annotations in the default values.yaml.
| RERANKER_ENABLED: "true" | ||
| RERANKER_TOP_K: "4" | ||
| RERANKER_BASE_URL: "http://{{ .Release.Name }}-reranker:{{ .Values.reranker.servicePort }}" | ||
| RERANKER_BASE_URL: '{{ if .Values.reranker.externalUrl }}{{ .Values.reranker.externalUrl }}{{ else }}http://{{ .Release.Name }}-reranker:{{ .Values.reranker.servicePort }}{{ end }}' |
There was a problem hiding this comment.
Disabling the reranker leaves the app pointing at it.
When reranker.enabled is false and externalUrl is empty, the reranker Deployment is not created, but the config still sets RERANKER_ENABLED to true and RERANKER_BASE_URL to the in-cluster reranker service. The app will keep calling a service that does not exist. Please tie RERANKER_ENABLED and RERANKER_BASE_URL to reranker.enabled and externalUrl, so turning the reranker off actually turns it off.
| # values.yaml is not templated, so set this manually when your release is not named "rag": | ||
| # --set "vllm.servingEngineSpec.modelSpec[0].hf_token.secretName=<release>-env-secrets" | ||
| # (repeat for indices 1, 2, 3 — see NOTES after install for the exact commands) | ||
| hfTokenSecretName: &vllmHfTokenSecretName "rag-env-secrets" |
There was a problem hiding this comment.
vLLM looks for the wrong secret name unless the release is called "rag".
hfTokenSecretName is fixed to rag-env-secrets, but the chart now names its secret <release>-env-secrets. So if the release is not called rag, the vLLM pods look for a secret that is not there and cannot read HF_TOKEN. The same hardcoded rag-env-secrets shows up four times in values-linagora.yaml. The post-install NOTES warn about this, but it is easy to miss. At minimum the comment should make clear you must change this to match your release name.
| serviceAccountName: "" | ||
| podSecurityContext: {} | ||
| nodeSelector: {} | ||
| resources: {} # e.g. limits: {"nvidia.com/gpu": 1} |
There was a problem hiding this comment.
The ray block sets resources twice.
Line 40 has resources: {} with the GPU example comment, and a few lines down there is the real resources with head and worker. YAML keeps only the last one, so this line and its comment do nothing, and stricter linters will fail on the repeated key. Please remove this empty resources: {}.
| - kind: Deployment | ||
| name: {{ .Release.Name }}-openrag | ||
| - kind: Deployment | ||
| name: {{ .Release.Name }}-reranker |
There was a problem hiding this comment.
Vault restart target points at a reranker that may not exist.
The indexer-ui restart target below is wrapped in if .Values.indexerUi.enabled, but this reranker target is not. When reranker.enabled is false this still lists <release>-reranker, a Deployment that was never created, and the Vault operator will try to restart something that is not there. Please wrap this reranker target in if .Values.reranker.enabled the same way.
| env: | ||
| # Name of an existing K8s Secret to use as-is. | ||
| # When set, the chart creates no Secret/ExternalSecret/VaultStaticSecret resource. | ||
| existingSecret: "" |
There was a problem hiding this comment.
App and database passwords can drift apart with external secrets.
When you use externalSecret, vaultStaticSecret, or set existingSecret, the chart stops creating the env secret, so the app gets POSTGRES_PASSWORD from your external source. But the Postgres server is still started from postgresql.auth.password. The two values are no longer the same, so the app cannot log in to the database. Please document that in these modes you also have to point Postgres at the same secret (postgresql.auth.existingSecret) or set postgresql.auth.password to match.
|
|
||
| {{- else if eq $type "externalSecret" }} | ||
| --- | ||
| apiVersion: external-secrets.io/v1 |
There was a problem hiding this comment.
External Secrets API version.
This ExternalSecret uses external-secrets.io/v1. That version only exists in newer External Secrets Operator releases (0.14 and up). On older installs only v1beta1 is served and the apply will fail. Worth noting a minimum operator version in the docs.
Ahmath-Gadji
left a comment
Ahmath-Gadji
left a comment
There was a problem hiding this comment.
Did a full pass over the chart changes. The move to release-scoped naming and configurable securityContexts is a good direction. My main concern is that the rename from the hardcoded rag- prefix to the fullname helper is only half-applied in a few places, which breaks things for any release not named rag, plus a couple of ingress/secret wiring issues. Notes inline, grouped roughly by impact.
Likely blockers:
- Ray worker PVC names still use
.Release.Name, so workers can't find their PVCs and stay Pending - Ray ingress points at the unauthenticated dashboard port (8265) and is enabled by default
- openrag and ray ingresses are both default-on with the same host and path
- vLLM
hf_tokensecret name hardcoded torag-env-secrets - dead top-level
ingress:block in values-linagora.yaml reranker.externalUrlis documented but not implemented- PVCs lost the
resource-policy: keepannotation, so uninstall now deletes the data - GPU resource request dropped from the Ray cluster
Smaller items and nits are inline too. Happy to help with any of these.
| - name: model-weights | ||
| persistentVolumeClaim: | ||
| claimName: rag-model-weights | ||
| claimName: {{ $.Release.Name }}-model-weights |
There was a problem hiding this comment.
These worker PVC claim names use {{ $.Release.Name }}, but the PVCs themselves and the head group just above both use the fullname helper. So for a release named rag, these resolve to rag-model-weights while the actual PVC is rag-openrag-stack-model-weights, and the workers can't mount it (they stay stuck Pending). Looks like the rename landed everywhere except these four. Could we switch them to {{ include "openrag-stack.fullname" $ }}-... so they match the head group?
| service: | ||
| name: {{ include "openrag-stack.fullname" . }}-raycluster-head-svc | ||
| port: | ||
| number: {{ .Values.env.config.RAY_DASHBOARD_PORT }} |
There was a problem hiding this comment.
Heads up, this points the ingress at the Ray dashboard port (8265), which has no auth and lets anyone submit jobs (effectively RCE on the cluster). The old ingress routed to the Serve port, not the dashboard. Since ray.ingress.enabled defaults to true, this would publish the dashboard on any cluster where the NetworkPolicy isn't enforced. Can we default ray.ingress.enabled to false and avoid exposing 8265 here?
| cpu: "4" | ||
| memory: "8Gi" | ||
| ingress: | ||
| enabled: true |
There was a problem hiding this comment.
With both openrag.ingress and ray.ingress defaulting to enabled, and both using host: "" and path: /, they'll collide on the same host and path on a single ingress controller. Might be worth defaulting one of them off, or requiring a host before they render.
| # values.yaml is not templated, so set this manually when your release is not named "rag": | ||
| # --set "vllm.servingEngineSpec.modelSpec[0].hf_token.secretName=<release>-env-secrets" | ||
| # (repeat for indices 1, 2, 3 — see NOTES after install for the exact commands) | ||
| hfTokenSecretName: &vllmHfTokenSecretName "rag-env-secrets" |
There was a problem hiding this comment.
Now that the env secret is named per release (<release>-env-secrets), this hardcoded rag-env-secrets only matches when the release is literally named rag. For any other name the vLLM pods can't find the HF token secret and gated model pulls fail. It used to work for any release name before the secret got renamed. The NOTES output already detects this and prints a workaround, but it would be nicer to wire the real secret name in here so it just works. Could we template it from the release name?
| operator: In | ||
| values: ["vlm"] | ||
|
|
||
| ingress: |
There was a problem hiding this comment.
I don't think this top-level ingress: block is read anywhere anymore. The chart moved to per-service keys (openrag.ingress.* and friends), so the nginx class, the rewrite, and the 500m body-size limit here are silently ignored. Looks like it needs to move under openrag.ingress.{className,annotations}. Worth dropping the /$1 rewrite too, since the routes now use path: / with no capture group.
| {{ $key }}: "{{ tpl (printf "%v" $value) $ }}" | ||
| {{- end }} | ||
| {{- if .Values.proxy.enabled }} | ||
| HTTP_PROXY: {{ .Values.proxy.url | quote }} |
There was a problem hiding this comment.
nit: if the proxy URL ever includes credentials, they'd end up in this plaintext ConfigMap. And if someone also sets HTTP_PROXY in env.config, you'd get a duplicate key in the same ConfigMap. Fine for the default, just flagging it.
| # incremented each time you make changes to the application. Versions are not expected to | ||
| # follow Semantic Versioning. They should reflect the version the application is using. | ||
| # It is recommended to use it with quotes. | ||
| version: 0.5.0 |
There was a problem hiding this comment.
nit: while bumping the chart version here, it would be a good moment to bump appVersion too. It's still 1.1.3 while the default images are on v1.1.12.
| vLLM serving pods will fail to mount HF_TOKEN unless you override the four | ||
| hf_token.secretName entries. Re-run the install/upgrade with: | ||
|
|
||
| helm upgrade {{ .Release.Name }} ./openrag-stack --reuse-values \ |
There was a problem hiding this comment.
nit: this hardcodes ./openrag-stack as the chart path, which won't be right for anyone installing from a repo (for example helm install ... linagora/openrag-stack).
| @@ -0,0 +1,4 @@ | |||
| {{- range .Values.extraObjects }} | |||
There was a problem hiding this comment.
nit: an empty or null entry in extraObjects would render as --- followed by null and fail to apply. A {{- if . }} guard inside the range would cover it.
| labels: | ||
| app.kubernetes.io/name: indexer-ui | ||
| app.kubernetes.io/instance: {{ .Release.Name }} | ||
| app.kubernetes.io/instance: {{ include "openrag-stack.fullname" . }} |
There was a problem hiding this comment.
nit: app.kubernetes.io/instance is set to the fullname here, but the shared selectorLabels helper uses .Release.Name. Minor label inconsistency across the stack (same pattern in infinity.yaml).
5 participants
Summary by CodeRabbit
New Features
Chores