Repository navigation
fix(helm): quote custom labels and fix default securityContext - #862
Conversation
What changed:
- deploy/helm/smg/templates/_helpers.tpl:
- --eviction-interval-secs → --eviction-interval (field: eviction_interval, main.rs:169)
- --postgres-pool-max → --postgres-pool-max-size (field: postgres_pool_max_size, main.rs:529)
- --redis-pool-max → --redis-pool-max-size (field: redis_pool_max_size, main.rs:538)
- --oracle-connect-descriptor → --oracle-dsn (field: oracle_dsn, main.rs:491)
- --oracle-username → --oracle-user (field: oracle_user, main.rs:495)
- deploy/helm/smg/Chart.yaml:
- appVersion: "0.7.0" → "1.3.3" (latest release tag v1.3.3)
- deploy/helm/smg/values.schema.json:
- Added missing policy enum values: consistent_hashing, prefix_hash, bucket
(all present in PolicyConfig enum in config/types.rs:247-320)
- deploy/helm/smg/values.yaml:
- evictionIntervalSecs: 60 → 120 (matches default_value_t = 120, main.rs:168)
- Updated policy comment to list all 8 valid values
Why:
The Helm chart generated incorrect CLI flags that would cause the SMG
binary to reject them at startup. Five flag names did not match the
actual clap field names in CliArgs, the eviction interval default was
wrong, the appVersion was stale, and three routing policies were
missing from the schema validation.
How:
Cross-referenced every --flag-name in _helpers.tpl against the CliArgs
struct fields in model_gateway/src/main.rs (clap converts field
snake_case to --kebab-case). Verified all default values in values.yaml
against default_value_t annotations. Checked PolicyConfig enum variants
in config/types.rs against values.schema.json. Validated with helm lint
and helm template rendering for all backend types (postgres, redis,
oracle).
Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
The parse_policy function in main.rs (lines 795-826) lacks match arms for consistent_hashing and bucket, causing them to silently fall back to round_robin. Until the application code is fixed, excluding these from the Helm schema prevents users from selecting non-functional policies. prefix_hash is retained as it is correctly handled. Addresses review feedback from Gemini Code Assist and CodeRabbit. Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
Custom labels on ServiceMonitor and Grafana dashboard ConfigMap were rendered via toYaml, which does not quote numeric-like values (e.g. grafana_dashboard: 1). Kubernetes rejects non-string label values, causing installation failures when users pass --set with numeric labels. Changed label rendering to use range with explicit quoting so that values like "1" are always emitted as strings. Also changed the default securityContext.runAsNonRoot from true to false because the lightseekorg/smg image runs as root, which caused CreateContainerConfigError on every fresh install. What changed: - deploy/helm/smg/templates/grafana-dashboard.yaml: replace toYaml with range+quote for custom labels - deploy/helm/smg/templates/servicemonitor.yaml: same label quoting fix - deploy/helm/smg/values.yaml: set runAsNonRoot default to false Signed-off-by: Simon Olin <simon@lightseek.org> Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
📝 WalkthroughWalkthroughUpdated Helm chart version to 1.3.3, renamed router and history CLI flags to new option names, added Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Summary of ChangesHello, 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 enhances the robustness and compatibility of the Helm chart by addressing two critical configuration issues. It ensures that custom labels are correctly formatted for Kubernetes, preventing deployment failures, and adjusts the default security context to align with the application's runtime requirements, facilitating smoother deployments. 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. Footnotes
|
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@deploy/helm/smg/Chart.yaml`:
- Around line 5-6: Bump the Chart.yaml chart version to reflect the appVersion
change: update the version field (currently "0.1.0") to a higher SemVer (e.g.,
"0.1.1" or appropriate minor/patch bump) so users are signaled about the
breaking/default value changes; modify the version key in Chart.yaml adjacent to
the existing appVersion key to a new SemVer value.
In `@deploy/helm/smg/values.yaml`:
- Around line 155-158: Add an inline comment next to the securityContext
configuration explaining why runAsNonRoot is set to false: document that the
container image "lightseekorg/smg" currently requires root to start (it triggers
CreateContainerConfigError when enforced), note any known limitations or plans
to upstream a non-root build, and reference any tickets or TODOs for converting
the image to non-root in the future; update the comment near the
securityContext/runAsNonRoot setting so future maintainers understand the
constraint and mitigation plan.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 21ec19a2-a29b-4b93-a4ef-e9fa2266a0cc
📒 Files selected for processing (6)
deploy/helm/smg/Chart.yamldeploy/helm/smg/templates/_helpers.tpldeploy/helm/smg/templates/grafana-dashboard.yamldeploy/helm/smg/templates/servicemonitor.yamldeploy/helm/smg/values.schema.jsondeploy/helm/smg/values.yaml
| version: 0.1.0 | ||
| appVersion: "0.7.0" | ||
| appVersion: "1.3.3" |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider bumping chart version alongside appVersion.
The appVersion bump to 1.3.3 reflects CLI flag renames (breaking changes for users with custom values). While version: 0.1.0 can remain if chart structure is unchanged, SemVer best practice for Helm charts is to bump the chart version when default values or template behavior changes, as this signals to users that an upgrade may require values file updates.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@deploy/helm/smg/Chart.yaml` around lines 5 - 6, Bump the Chart.yaml chart
version to reflect the appVersion change: update the version field (currently
"0.1.0") to a higher SemVer (e.g., "0.1.1" or appropriate minor/patch bump) so
users are signaled about the breaking/default value changes; modify the version
key in Chart.yaml adjacent to the existing appVersion key to a new SemVer value.
| securityContext: | ||
| readOnlyRootFilesystem: true | ||
| runAsNonRoot: true | ||
| runAsNonRoot: false | ||
| allowPrivilegeEscalation: false |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Document the rationale for runAsNonRoot: false.
The change fixes the CreateContainerConfigError since lightseekorg/smg runs as root. However, this weakens the default security posture. Consider adding a comment explaining why root is required, so future maintainers understand the constraint:
# -- Security context for router container
+ # NOTE: runAsNonRoot is false because the lightseekorg/smg image currently
+ # runs as root. Set to true if using a non-root image variant.
securityContext:
readOnlyRootFilesystem: true
runAsNonRoot: false
allowPrivilegeEscalation: false🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@deploy/helm/smg/values.yaml` around lines 155 - 158, Add an inline comment
next to the securityContext configuration explaining why runAsNonRoot is set to
false: document that the container image "lightseekorg/smg" currently requires
root to start (it triggers CreateContainerConfigError when enforced), note any
known limitations or plans to upstream a non-root build, and reference any
tickets or TODOs for converting the image to non-root in the future; update the
comment near the securityContext/runAsNonRoot setting so future maintainers
understand the constraint and mitigation plan.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the SMG Helm chart, incrementing the application version to 1.3.3. It renames several command-line arguments for the router, including --eviction-interval-secs to --eviction-interval and various database pool size and Oracle connection parameters. The templating for Grafana dashboard and ServiceMonitor labels has been refined, and a new prefix_hash routing policy has been introduced. Additionally, the default evictionIntervalSecs has been increased to 120, and the router container's runAsNonRoot security context has been set to false.
The chart's default securityContext sets readOnlyRootFilesystem: true, which prevents the container from writing to any path in the image filesystem. Python and other runtime components may need to write temporary files to /tmp. This was discovered during Helm chart testing against a live cluster where the pod would fail to start or crash when the process attempted to write temp files. The workaround branches (fix/helm-edge-cases, fix/helm-minimal-deploy, fix/helm-observability) included this fix alongside a runAsNonRoot: false change. Now that the Docker image runs as non-root (previous commit), runAsNonRoot: true is correct and this /tmp volume is the only remaining piece needed. What changed: - deploy/helm/smg/templates/deployment-router.yaml: added emptyDir volume "tmp" mounted at /tmp in the router container Why: - readOnlyRootFilesystem: true is a security best practice but requires writable mounts for directories that need runtime writes - /tmp is the standard location for temporary files in Python processes Refs: #860, #861, #862 Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
1 participant
Description
Problem
Two issues discovered during observability testing of the Helm chart:
Custom labels on ServiceMonitor and Grafana dashboard ConfigMap fail with
--set: ThetoYamlhelper renders numeric-like values without quoting (e.g.grafana_dashboard: 1instead ofgrafana_dashboard: "1"). Kubernetes requires label values to be strings, so this causesunable to decode: json: cannot unmarshal number into Go struct field ObjectMeta.metadata.labels of type string.Default
securityContext.runAsNonRoot: trueis incompatible with the SMG image: Thelightseekorg/smgimage runs as root, causingCreateContainerConfigErroron every fresh install with default values.Solution
toYamllabel rendering withrange+quoteto ensure all custom label values are always emitted as strings, regardless of whether they look numericrunAsNonRootfromtruetofalseto match the image's runtime userChanges
deploy/helm/smg/templates/grafana-dashboard.yaml: replacetoYamlwithrange+quotefor custom labelsdeploy/helm/smg/templates/servicemonitor.yaml: same label quoting fixdeploy/helm/smg/values.yaml: setrunAsNonRootdefault tofalseTest Plan
Full observability test suite run against
helm-test-observabilitynamespace on the dev cluster:grafana_dashboard: "1"label--log-json,--log-level debug,--prometheus-port 29000--enable-traceand--otlp-traces-endpointNamespace cleaned up after testing (confirmed NotFound).
Checklist
Summary by CodeRabbit
New Features
prefix_hashas a new router load-balancing policy option.Chores