Repository navigation
Harden Helm deployment defaults - #589
Conversation
|
Warning Review limit reached
More reviews will be available in 46 minutes and 33 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis PR adds comprehensive Kubernetes security and networking resources to the Helm chart, including ServiceAccount isolation (no RBAC, no token mounting), NetworkPolicy with configurable ingress/egress rules (DNS, public HTTPS with private range exclusions), and optional HTTPRoute for Gateway API routing. Pod spec now explicitly sets these security fields. Tests validate all defaults and customization paths, and the operator runbook provides deployment guidance with Helm examples. ChangesKubernetes Security & Networking
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
coding-agent/docs/deployment/phase2-k8s.md (1)
141-150:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't use the
orbstackoverlay in the cluster install example.This still layers
helm/values-orbstack-kimi.yamlinto the shared/public-cluster runbook, which conflicts with the warning above that Orbstack values are local-dev only. A reader can copy this verbatim and inherit unsafe defaults into a real cluster. Please switch this example to a production-safe overlay or document every override it depends on.🤖 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 `@coding-agent/docs/deployment/phase2-k8s.md` around lines 141 - 150, The helm example uses the local-dev Orbstack overlay (helm/values-orbstack-kimi.yaml) which is unsafe for cluster installs; change the command to reference a production-safe values file (for example replace helm/values-orbstack-kimi.yaml with a production overlay like helm/values-production.yaml or helm/values-k8s.yaml) or remove the -f flag and instead explicitly set every production-safe override shown (server.auth.enabled, server.allowUnauthenticated, image.* and resource limits/requests); if you keep an overlay, update or add documentation listing every override that overlay provides so readers know what defaults it changes.
🧹 Nitpick comments (1)
coding-agent/helm/templates/networkpolicy.yaml (1)
46-55: 💤 Low valueConsider conditionally rendering the
exceptfield.The
exceptfield in theipBlock(lines 50-51) is always rendered even if the list is empty. Whileexcept: []is valid YAML, it would be cleaner to conditionally include the field only when the list is non-empty:♻️ Optional refactor for cleaner YAML
- ipBlock: cidr: {{ .Values.networkPolicy.egress.publicHttps.cidr | quote }} + {{- if .Values.networkPolicy.egress.publicHttps.except }} except: {{- toYaml .Values.networkPolicy.egress.publicHttps.except | nindent 14 }} + {{- 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 `@coding-agent/helm/templates/networkpolicy.yaml` around lines 46 - 55, The ipBlock's except field is rendered even when .Values.networkPolicy.egress.publicHttps.except is empty; update the networkpolicy.yaml template so the ipBlock includes the except: block only when the list has items by wrapping the existing "except: {{- toYaml .Values.networkPolicy.egress.publicHttps.except | nindent 14 }}" rendering in a conditional that checks the list (e.g., using an if with .Values.networkPolicy.egress.publicHttps.except or the len function) so empty arrays are not emitted while keeping the rest of the ipBlock and ports logic under the existing .Values.networkPolicy.egress.publicHttps.enabled check.
🤖 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 `@coding-agent/helm/templates/networkpolicy.yaml`:
- Around line 24-27: The template currently uses "{{- with
.Values.networkPolicy.ingress.from }}" which treats empty slices as falsy and
omits the "from:" field, changing NetworkPolicy semantics; change the template
to preserve empty-array semantics by either (A) replacing the with with an
explicit length check like "{{- if gt (len .Values.networkPolicy.ingress.from) 0
}}" to only render a populated from: block, or (B) use an "if" that treats empty
slices as truthy (e.g., "{{- if .Values.networkPolicy.ingress.from }}" is not
safe) — recommended fix: replace the "{{- with
.Values.networkPolicy.ingress.from }}" block with "{{- if or (not
.Values.networkPolicy.ingress.from) (gt (len .Values.networkPolicy.ingress.from)
0) }}" then render "from:" and the toYaml content so that an explicit empty
array (from: []) is emitted when provided, while still allowing omission when
the value is unset or null; update the "from:" rendering around the toYaml call
accordingly.
- Around line 12-18: The template currently renders spec.policyTypes even when
both .Values.networkPolicy.ingress.enabled and
.Values.networkPolicy.egress.enabled are false, producing an empty policyTypes
list; update the template to avoid this by gating rendering: either wrap the
entire NetworkPolicy resource in an if that checks
(.Values.networkPolicy.ingress.enabled or .Values.networkPolicy.egress.enabled)
so the resource is omitted when neither is enabled, or at minimum wrap the
spec.policyTypes section with that same conditional so policyTypes is omitted
when both are false; reference the existing symbols
.Values.networkPolicy.enabled, .Values.networkPolicy.ingress.enabled,
.Values.networkPolicy.egress.enabled and the spec.policyTypes block to locate
where to add the conditional.
---
Outside diff comments:
In `@coding-agent/docs/deployment/phase2-k8s.md`:
- Around line 141-150: The helm example uses the local-dev Orbstack overlay
(helm/values-orbstack-kimi.yaml) which is unsafe for cluster installs; change
the command to reference a production-safe values file (for example replace
helm/values-orbstack-kimi.yaml with a production overlay like
helm/values-production.yaml or helm/values-k8s.yaml) or remove the -f flag and
instead explicitly set every production-safe override shown
(server.auth.enabled, server.allowUnauthenticated, image.* and resource
limits/requests); if you keep an overlay, update or add documentation listing
every override that overlay provides so readers know what defaults it changes.
---
Nitpick comments:
In `@coding-agent/helm/templates/networkpolicy.yaml`:
- Around line 46-55: The ipBlock's except field is rendered even when
.Values.networkPolicy.egress.publicHttps.except is empty; update the
networkpolicy.yaml template so the ipBlock includes the except: block only when
the list has items by wrapping the existing "except: {{- toYaml
.Values.networkPolicy.egress.publicHttps.except | nindent 14 }}" rendering in a
conditional that checks the list (e.g., using an if with
.Values.networkPolicy.egress.publicHttps.except or the len function) so empty
arrays are not emitted while keeping the rest of the ipBlock and ports logic
under the existing .Values.networkPolicy.egress.publicHttps.enabled check.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 8ff32ec6-22b0-4d31-bb5d-3f1a868afd57
📒 Files selected for processing (8)
coding-agent/docs/deployment/phase2-k8s.mdcoding-agent/helm/templates/_helpers.tplcoding-agent/helm/templates/deployment.yamlcoding-agent/helm/templates/httproute.yamlcoding-agent/helm/templates/networkpolicy.yamlcoding-agent/helm/templates/serviceaccount.yamlcoding-agent/helm/values.yamlcoding-agent/tests/deploy/test_helm_chart.py
Summary
Tests
Summary by CodeRabbit
Documentation
New Features