-
Notifications
You must be signed in to change notification settings - Fork 6
feat : add helm chart #37
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Summary of ChangesHello @Chesblaw, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the deployability of the Helios L7 reverse proxy and load balancer by introducing a dedicated Helm chart. This chart streamlines the process of deploying Helios onto a Kubernetes cluster, providing a complete set of Kubernetes manifests and a comprehensive Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
The pull request introduces a Helm chart for the Helios application. The changes include adding Chart.yaml, _helpers.tpl, configmap.yaml, deployment.yaml, hpa.yaml, ingress.yaml, service.yaml, and values.yaml files to the helm/helios directory. A minor modification was also made to the README.md file. I have identified a potential issue in the README.md file that needs to be addressed.
| - **Plugin Middleware**: Configurable middleware chain (built-ins: logging, headers) | ||
|
|
||
| ## Performance Benchmarks | ||
| ## Performance Benchmarkshttps://github.com/Chesblaw/Helios.git |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The URL https://github.com/Chesblaw/Helios.git is appended directly to the heading "Performance Benchmarks". This looks unintentional and should be removed or placed correctly.
It's likely that this was copy/pasted from somewhere and inadvertently included in the heading.
| ## Performance Benchmarkshttps://github.com/Chesblaw/Helios.git | |
| ## Performance Benchmarks |
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Summary
- Adds Helm chart under
helm/helios/**(Chart.yaml, templates, values.yaml). - README also changed but appears out-of-sync with current main. Please keep this PR focused on the chart.
What needs improvement
- ConfigMap is incomplete
- Only
server.portandtls.enabledare rendered. Other sections from values (backends, load_balancer, health_checks, rate_limit, circuit_breaker, adminAPI, metrics, plugins) are missing. This will result in Helios running with an incomplete config.
- Service targetPort mismatch
- Template references
.Values.service.targetPortbut it does not exist invalues.yaml. Prefer named container ports and reference them from the Service.
- Probes should hit the main HTTP port
- Liveness/readiness currently point to
metrics.port. If metrics are disabled, probes break. Probe the main HTTP service (http).
- Ensure the binary actually reads the mounted config
- ConfigMap is mounted at
/etc/helios/helios.yaml, but the container doesn’t set a working directory or explicit args to point to it. Either setworkingDir: /etc/heliosor pass an explicit flag if the binary supports it.
- Security hardening
- Add a container
securityContext(runAsNonRoot, readOnlyRootFilesystem, no privilege escalation, drop all capabilities).
- Ingress className should be conditional
- Don’t render an empty
ingressClassNamewhen the value is empty.
- Secrets for admin token
adminAPI.auth_tokenshould come from a Secret, not values/ConfigMap directly.
- Rollout on config change
- Add a checksum annotation of the ConfigMap to the Pod template to trigger rolling updates when config changes.
- Image tag/policy
- Avoid
latestfor production. Alignimage.tagwithappVersion, or set pull policy accordingly.
- Resources & HPA
- Provide default
resources.requests/limits. HPA on CPU requires requests to be set for accurate scaling.
Concrete suggestions
- ConfigMap (render a complete
helios.yaml):
data:
helios.yaml: |
server:
port: {{ .Values.service.port }}
tls:
enabled: {{ .Values.tls.enabled }}
{{- if .Values.tls.enabled }}
certFile: {{ .Values.certFile | quote }}
keyFile: {{ .Values.keyFile | quote }}
{{- end }}
backends:
{{- toYaml .Values.backends | nindent 6 }}
load_balancer:
strategy: {{ .Values.load_balancer.strategy | quote }}
health_checks:
{{- toYaml .Values.health_checks | nindent 6 }}
rate_limit:
{{- toYaml .Values.rate_limit | nindent 6 }}
circuit_breaker:
{{- toYaml .Values.circuit_breaker | nindent 6 }}
admin_api:
enabled: {{ .Values.adminAPI.enabled }}
port: {{ .Values.adminAPI.port }}
auth_token: {{ .Values.adminAPI.auth_token | quote }}
metrics:
enabled: {{ .Values.metrics.enabled }}
port: {{ .Values.metrics.port }}
path: {{ .Values.metrics.path | quote }}
plugins:
{{- toYaml .Values.plugins | nindent 6 }}- Service (use named ports and remove nonexistent targetPort):
ports:
- name: http
port: {{ .Values.service.port }}
targetPort: http
{{- if .Values.metrics.enabled }}
- name: metrics
port: {{ .Values.metrics.port }}
targetPort: metrics
{{- end }}
{{- if .Values.adminAPI.enabled }}
- name: admin
port: {{ .Values.adminAPI.port }}
targetPort: admin
{{- end }}- Deployment (workingDir, probes to http, guard resources, securityContext, config checksum):
metadata:
annotations:
checksum/config: {{ include (print $.Template.BasePath "/configmap.yaml") . | sha256sum }}
...
containers:
- name: helios
image: "{{ .Values.image.repository }}:{{ .Values.image.tag }}"
imagePullPolicy: {{ .Values.image.pullPolicy }}
command: ["/helios"]
workingDir: /etc/helios
ports:
- name: http
containerPort: {{ .Values.service.port }}
{{- if .Values.metrics.enabled }}
- name: metrics
containerPort: {{ .Values.metrics.port }}
{{- end }}
{{- if .Values.adminAPI.enabled }}
- name: admin
containerPort: {{ .Values.adminAPI.port }}
{{- end }}
livenessProbe:
httpGet: { path: /health, port: http }
readinessProbe:
httpGet: { path: /health, port: http }
{{- if .Values.resources }}
resources:
{{- toYaml .Values.resources | nindent 6 }}
{{- end }}
securityContext:
runAsNonRoot: true
readOnlyRootFilesystem: true
allowPrivilegeEscalation: false
capabilities: { drop: ["ALL"] }- Ingress (conditional className):
{{- if .Values.ingress.className }}
ingressClassName: {{ .Values.ingress.className }}
{{- end }}- values.yaml (defaults & consistency):
image:
repository: "helios/helios"
tag: "{{ .Chart.AppVersion }}"
pullPolicy: IfNotPresent
resources:
requests:
cpu: 100m
memory: 128Mi
limits:
cpu: 500m
memory: 512MiProcess & docs
- Please sync/rebase with the latest
mainand drop unrelated README changes to keep this PR focused on the chart. We can add a Helm section in README or a separate doc after the chart is solid. - Link the PR to the issue by adding
Closes #31in the description. - Suggested checks:
helm lint helm/helios
helm template my-helios helm/helios --values helm/helios/values.yaml|
Quick follow-up on images (Issue #27 is still open)
Also noticed:
The rest of the requested changes remain the same (ConfigMap completeness, named ports + probes to |
|
Sure! |
|
@Chesblaw ping me if need help thank you 👍 |




No description provided.