Helm changes #5251
Helm changes #5251
Conversation
|
@IgorMilavec is attempting to deploy a commit to the Danswer Team on Vercel. A member of the Team first needs to authorize it. |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Greptile Summary
This PR introduces significant changes to make the Helm chart more enterprise-ready through several key architectural improvements:
Security Context Refactoring: The most substantial change moves all Celery workers from shared security contexts (celery_shared) to worker-specific security contexts (e.g., celery_worker_heavy, celery_worker_light, celery_worker_primary). This enables granular security policies per worker type, allowing enterprises to apply different security requirements based on each worker's specific function and risk profile.
Database Connectivity Flexibility: A new helper template onyx-stack.postgresHost is introduced in _helpers.tpl and used in configmap.yaml to support external PostgreSQL instances. This replaces the hardcoded {{ .Release.Name }}-postgresql pattern, allowing enterprises to use managed database services while maintaining backward compatibility with internal PostgreSQL deployments.
Ingress Modernization: Both API and webserver ingress templates are updated to use configurable annotations via values files instead of hardcoded annotations. The templates now support the modern ingressClassName field (Kubernetes 1.18+) while removing deprecated annotation patterns. This provides flexibility for different ingress controllers and certificate management solutions.
Template Simplification: Unused volume mount and volume configurations are systematically removed across multiple deployment templates (API, Slackbot, and various Celery workers). This cleanup reduces template complexity and eliminates potentially confusing configuration options that weren't being utilized.
Worker Specialization: Some Celery workers receive task redistribution changes, such as the document fetching worker having its concurrency increased from 2 to 12 threads and queue assignments modified to focus on specific tasks. The user files indexing worker gains threaded pool configuration optimized for file processing workloads.
Model Deployment Changes: The indexing and inference model deployments shift from security context-based configurations to service account-based permissions, aligning with enterprise RBAC practices.
These changes collectively provide more granular control, better security isolation, enhanced flexibility for external service integration, and cleaner configuration management - all crucial aspects for enterprise Kubernetes deployments.
Confidence score: 2/5
- This PR introduces several breaking changes and potential deployment issues that require careful review
- Score lowered due to missing configuration values in several templates that will cause rendering failures and potential breaking changes for existing deployments
- Pay close attention to celery-worker-monitoring.yaml and model deployment templates which reference undefined values
16 files reviewed, 3 comments
| volumes: | ||
| {{- toYaml . | nindent 8 }} | ||
| {{- end }} | ||
| --filename /tmp/onyx_k8s_beat_liveness.txt |
There was a problem hiding this comment.
style: File missing newline at end - should end with a newline character for consistency
| resources: | ||
| {{- toYaml .Values.indexCapability.resources | nindent 10 }} | ||
| {{- end }} No newline at end of file | ||
| {{- toYaml .Values.indexCapability.resources | nindent 12 }} No newline at end of file |
There was a problem hiding this comment.
logic: Resources are now unconditionally applied without checking if .Values.indexCapability.resources exists. This will cause template rendering to fail if resources are not defined in values.yaml.
| resources: | |
| {{- toYaml .Values.indexCapability.resources | nindent 10 }} | |
| {{- end }} | |
| \ No newline at end of file | |
| {{- toYaml .Values.indexCapability.resources | nindent 12 }} | |
| {{- if .Values.indexCapability.resources }} | |
| resources: | |
| {{- toYaml .Values.indexCapability.resources | nindent 12 }} | |
| {{- end }} |
| - path: /api(/|$)(.*) | ||
| pathType: Prefix | ||
| - path: {{ .Values.ingress.api.path | default "/api(/|$)(.*)" }} | ||
| pathType: {{ .Values.ingress.api.pathType | default "ImplementationSpecific" }} |
There was a problem hiding this comment.
style: PathType 'ImplementationSpecific' may not work with all ingress controllers. Consider 'Prefix' as the default for better compatibility.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
| - name: celery-worker-docfetching | ||
| securityContext: | ||
| {{- toYaml .Values.celery_shared.securityContext | nindent 12 }} | ||
| {{- toYaml .Values.celery_worker_docfetching.securityContext | nindent 12 }} |
There was a problem hiding this comment.
Container security context reference points to undefined field in values.yaml
Prompt for AI agents
Address the following comment on deployment/helm/charts/onyx/templates/celery-worker-docfetching.yaml at line 39:
<comment>Container security context reference points to undefined field in values.yaml</comment>
<file context>
@@ -32,11 +32,11 @@ spec:
{{- end }}
serviceAccountName: {{ include "onyx-stack.serviceAccountName" . }}
securityContext:
- {{- toYaml .Values.celery_shared.podSecurityContext | nindent 8 }}
+ {{- toYaml .Values.celery_worker_docfetching.podSecurityContext | nindent 8 }}
containers:
- name: celery-worker-docfetching
securityContext:
- {{- toYaml .Values.celery_shared.securityContext | nindent 12 }}
</file context>
| {{- toYaml .Values.celery_worker_docfetching.securityContext | nindent 12 }} | |
| {{- toYaml .Values.celery_shared.securityContext | nindent 12 }} |
| serviceAccountName: {{ include "onyx-stack.serviceAccountName" . }} | ||
| securityContext: | ||
| {{- toYaml .Values.celery_shared.podSecurityContext | nindent 8 }} | ||
| {{- toYaml .Values.celery_worker_docfetching.podSecurityContext | nindent 8 }} |
There was a problem hiding this comment.
Security context reference points to undefined field in values.yaml
Prompt for AI agents
Address the following comment on deployment/helm/charts/onyx/templates/celery-worker-docfetching.yaml at line 35:
<comment>Security context reference points to undefined field in values.yaml</comment>
<file context>
@@ -32,11 +32,11 @@ spec:
{{- end }}
serviceAccountName: {{ include "onyx-stack.serviceAccountName" . }}
securityContext:
- {{- toYaml .Values.celery_shared.podSecurityContext | nindent 8 }}
+ {{- toYaml .Values.celery_worker_docfetching.podSecurityContext | nindent 8 }}
containers:
- name: celery-worker-docfetching
</file context>
| {{- toYaml .Values.celery_worker_docfetching.podSecurityContext | nindent 8 }} | |
| {{- toYaml .Values.celery_shared.podSecurityContext | nindent 8 }} |
| serviceAccountName: {{ include "onyx-stack.serviceAccountName" . }} | ||
| securityContext: | ||
| {{- toYaml .Values.celery_shared.podSecurityContext | nindent 8 }} | ||
| {{- toYaml .Values.celery_worker_user_files_indexing.podSecurityContext | nindent 8 }} |
There was a problem hiding this comment.
Template references undefined security context values that don't exist in values.yaml
Prompt for AI agents
Address the following comment on deployment/helm/charts/onyx/templates/celery-worker-user-files-indexing.yaml at line 35:
<comment>Template references undefined security context values that don't exist in values.yaml</comment>
<file context>
@@ -32,11 +32,11 @@ spec:
{{- end }}
serviceAccountName: {{ include "onyx-stack.serviceAccountName" . }}
securityContext:
- {{- toYaml .Values.celery_shared.podSecurityContext | nindent 8 }}
+ {{- toYaml .Values.celery_worker_user_files_indexing.podSecurityContext | nindent 8 }}
containers:
- name: celery-worker-user-files-indexing
</file context>
| {{- toYaml .Values.celery_worker_user_files_indexing.podSecurityContext | nindent 8 }} | |
| {{- toYaml .Values.celery_shared.podSecurityContext | nindent 8 }} |
| - name: celery-worker-heavy | ||
| securityContext: | ||
| {{- toYaml .Values.celery_shared.securityContext | nindent 12 }} | ||
| {{- toYaml .Values.celery_worker_heavy.securityContext | nindent 12 }} |
There was a problem hiding this comment.
Template references undefined values: celery_worker_heavy.securityContext is not defined in values.yaml
Prompt for AI agents
Address the following comment on deployment/helm/charts/onyx/templates/celery-worker-heavy.yaml at line 39:
<comment>Template references undefined values: celery_worker_heavy.securityContext is not defined in values.yaml</comment>
<file context>
@@ -32,11 +32,11 @@ spec:
{{- end }}
serviceAccountName: {{ include "onyx-stack.serviceAccountName" . }}
securityContext:
- {{- toYaml .Values.celery_shared.podSecurityContext | nindent 8 }}
+ {{- toYaml .Values.celery_worker_heavy.podSecurityContext | nindent 8 }}
containers:
- name: celery-worker-heavy
securityContext:
- {{- toYaml .Values.celery_shared.securityContext | nindent 12 }}
</file context>
| {{- toYaml .Values.celery_worker_heavy.securityContext | nindent 12 }} | |
| {{- toYaml (.Values.celery_worker_heavy.securityContext | default .Values.celery_shared.securityContext) | nindent 12 }} |
| serviceAccountName: {{ include "onyx-stack.serviceAccountName" . }} | ||
| securityContext: | ||
| {{- toYaml .Values.celery_shared.podSecurityContext | nindent 8 }} | ||
| {{- toYaml .Values.celery_worker_heavy.podSecurityContext | nindent 8 }} |
There was a problem hiding this comment.
Template references undefined values: celery_worker_heavy.podSecurityContext is not defined in values.yaml
Prompt for AI agents
Address the following comment on deployment/helm/charts/onyx/templates/celery-worker-heavy.yaml at line 35:
<comment>Template references undefined values: celery_worker_heavy.podSecurityContext is not defined in values.yaml</comment>
<file context>
@@ -32,11 +32,11 @@ spec:
{{- end }}
serviceAccountName: {{ include "onyx-stack.serviceAccountName" . }}
securityContext:
- {{- toYaml .Values.celery_shared.podSecurityContext | nindent 8 }}
+ {{- toYaml .Values.celery_worker_heavy.podSecurityContext | nindent 8 }}
containers:
- name: celery-worker-heavy
</file context>
| {{- toYaml .Values.celery_worker_heavy.podSecurityContext | nindent 8 }} | |
| {{- toYaml (.Values.celery_worker_heavy.podSecurityContext | default .Values.celery_shared.podSecurityContext) | nindent 8 }} |
| serviceAccountName: {{ include "onyx-stack.serviceAccountName" . }} | ||
| securityContext: | ||
| {{- toYaml .Values.celery_shared.podSecurityContext | nindent 8 }} | ||
| {{- toYaml .Values.celery_worker_monitoring.podSecurityContext | nindent 8 }} |
There was a problem hiding this comment.
Template references undefined value .Values.celery_worker_monitoring.podSecurityContext
Prompt for AI agents
Address the following comment on deployment/helm/charts/onyx/templates/celery-worker-monitoring.yaml at line 35:
<comment>Template references undefined value .Values.celery_worker_monitoring.podSecurityContext</comment>
<file context>
@@ -32,11 +32,11 @@ spec:
{{- end }}
serviceAccountName: {{ include "onyx-stack.serviceAccountName" . }}
securityContext:
- {{- toYaml .Values.celery_shared.podSecurityContext | nindent 8 }}
+ {{- toYaml .Values.celery_worker_monitoring.podSecurityContext | nindent 8 }}
containers:
- name: celery-worker-monitoring
</file context>
| {{- toYaml .Values.celery_worker_monitoring.podSecurityContext | nindent 8 }} | |
| {{- toYaml .Values.celery_shared.podSecurityContext | nindent 8 }} |
| volumes: | ||
| {{- toYaml . | nindent 8 }} | ||
| {{- end }} | ||
| --filename /tmp/onyx_k8s_beat_liveness.txt |
There was a problem hiding this comment.
File should end with a newline character
Prompt for AI agents
Address the following comment on deployment/helm/charts/onyx/templates/celery-beat.yaml at line 76:
<comment>File should end with a newline character</comment>
<file context>
@@ -77,8 +73,4 @@ spec:
- >
python onyx/background/celery/celery_k8s_probe.py
--probe liveness
- --filename /tmp/onyx_k8s_beat_liveness.txt
- {{- with .Values.celery_beat.volumes }}
- volumes:
- {{- toYaml . | nindent 8 }}
- {{- end }}
+ --filename /tmp/onyx_k8s_beat_liveness.txt
</file context>
| --filename /tmp/onyx_k8s_beat_liveness.txt | |
| --filename /tmp/onyx_k8s_beat_liveness.txt |
| - name: celery-beat | ||
| securityContext: | ||
| {{- toYaml .Values.celery_shared.securityContext | nindent 12 }} | ||
| {{- toYaml .Values.celery_beat.securityContext | nindent 12 }} |
There was a problem hiding this comment.
Template references undefined value .Values.celery_beat.securityContext
Prompt for AI agents
Address the following comment on deployment/helm/charts/onyx/templates/celery-beat.yaml at line 37:
<comment>Template references undefined value .Values.celery_beat.securityContext</comment>
<file context>
@@ -30,11 +30,11 @@ spec:
{{- end }}
serviceAccountName: {{ include "onyx-stack.serviceAccountName" . }}
securityContext:
- {{- toYaml .Values.celery_shared.podSecurityContext | nindent 8 }}
+ {{- toYaml .Values.celery_beat.podSecurityContext | nindent 8 }}
containers:
- name: celery-beat
securityContext:
- {{- toYaml .Values.celery_shared.securityContext | nindent 12 }}
</file context>
| serviceAccountName: {{ include "onyx-stack.serviceAccountName" . }} | ||
| securityContext: | ||
| {{- toYaml .Values.celery_shared.podSecurityContext | nindent 8 }} | ||
| {{- toYaml .Values.celery_beat.podSecurityContext | nindent 8 }} |
There was a problem hiding this comment.
Template references undefined values .Values.celery_beat.podSecurityContext and .Values.celery_beat.securityContext
Prompt for AI agents
Address the following comment on deployment/helm/charts/onyx/templates/celery-beat.yaml at line 33:
<comment>Template references undefined values .Values.celery_beat.podSecurityContext and .Values.celery_beat.securityContext</comment>
<file context>
@@ -30,11 +30,11 @@ spec:
{{- end }}
serviceAccountName: {{ include "onyx-stack.serviceAccountName" . }}
securityContext:
- {{- toYaml .Values.celery_shared.podSecurityContext | nindent 8 }}
+ {{- toYaml .Values.celery_beat.podSecurityContext | nindent 8 }}
containers:
- name: celery-beat
</file context>
| {{- if .Values.inferenceCapability.podSecurityContext }} | ||
| securityContext: | ||
| {{- toYaml .Values.inferenceCapability.podSecurityContext | nindent 8 }} | ||
| {{- if .Values.inferenceCapability.serviceAccountName }} |
There was a problem hiding this comment.
Removing security contexts may compromise enterprise security requirements. Consider keeping both pod and container security contexts configurable.
Prompt for AI agents
Address the following comment on deployment/helm/charts/onyx/templates/inference-model-deployment.yaml at line 23:
<comment>Removing security contexts may compromise enterprise security requirements. Consider keeping both pod and container security contexts configurable.</comment>
<file context>
@@ -20,9 +20,8 @@ spec:
{{ .key }}: {{ .value }}
{{- end }}
spec:
- {{- if .Values.inferenceCapability.podSecurityContext }}
- securityContext:
- {{- toYaml .Values.inferenceCapability.podSecurityContext | nindent 8 }}
+ {{- if .Values.inferenceCapability.serviceAccountName }}
+ serviceAccountName: {{ .Values.inferenceCapability.serviceAccountName }}
{{- end }}
</file context>
Description
This is a PR to discuss possible changes to make Helm chart more enterprise ready
How Has This Been Tested?
We have used this to deploy to our environments
Backporting (check the box to trigger backport action)
Note: You have to check that the action passes, otherwise resolve the conflicts manually and tag the patches.
Summary by cubic
Make the Helm chart more enterprise-ready with flexible ingress settings, external Postgres support, per-worker security controls, and a dedicated user-files-indexing worker. Also adds service account overrides for model pods and simplifies templates.
New Features
Migration