[APIP] Update helm charts#2723
Conversation
|
Warning Review limit reached
Next review available in: 58 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughChangesThe Gateway Helm chart adds controller, router, policy-engine, and application configuration values with corresponding template output. Chart metadata advances to Gateway Helm configuration
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ 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.
🧹 Nitpick comments (3)
kubernetes/helm/gateway-helm-chart/templates/gateway/gateway-config.yaml (2)
191-197: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winPrevent potential rendering panic when
request_transformationis omitted.If
router.luais overridden and contains other settings but omitsrequest_transformation, evaluating$router.lua.request_transformation.script_pathwill cause a Helm nil-pointer panic. Wrapping the block in a check forrequest_transformationensures safe rendering.♻️ Proposed refactor
{{- if $router.lua }} + {{- if $router.lua.request_transformation }} [router.lua.request_transformation] {{- if $router.lua.request_transformation.script_path }} script_path = {{ $router.lua.request_transformation.script_path | quote }} {{- end }} {{- end }} + {{- 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 `@kubernetes/helm/gateway-helm-chart/templates/gateway/gateway-config.yaml` around lines 191 - 197, Guard the request-transformation block in the router.lua configuration template by checking that $router.lua.request_transformation exists before accessing its script_path field. Preserve the existing script_path output when request_transformation is present, while allowing rendering when router.lua contains other settings without it.
433-437: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrevent invalid TOML if
enable_validationis omitted.Since the
subscriptionsblock is commented out invalues.yaml, it does not have a default structure to merge against. If a user definessubscriptions: {}in their custom values without explicitly settingenable_validation, the template will renderenable_validation = <no value>, resulting in invalid TOML syntax. Adding adefaultfallback ensures the generated configuration is always valid.♻️ Proposed refactor
{{- if .Values.gateway.config.subscriptions }} [subscriptions] - enable_validation = {{ .Values.gateway.config.subscriptions.enable_validation }} + enable_validation = {{ .Values.gateway.config.subscriptions.enable_validation | default false }} {{- 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 `@kubernetes/helm/gateway-helm-chart/templates/gateway/gateway-config.yaml` around lines 433 - 437, Update the subscriptions block in the gateway configuration template to apply a boolean default when .Values.gateway.config.subscriptions.enable_validation is omitted. Preserve explicitly provided values while ensuring subscriptions: {} renders valid TOML rather than an empty value.kubernetes/helm/gateway-helm-chart/values.yaml (1)
309-327: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueInclude missing field in
text_format.The
json_fieldsmapping includes%RESP(X-ENVOY-UPSTREAM-SERVICE-TIME)%asupSvcT, but this field is omitted from thetext_formattemplate. Consider adding it totext_formatfor consistency between log formats.♻️ Proposed refactor
# Text format template - used when format is "text" # Uses Envoy command operators: https://www.envoyproxy.io/docs/envoy/latest/configuration/observability/access_log/usage text_format: | - [%START_TIME%] "%REQ(:METHOD)% %REQ(X-ENVOY-ORIGINAL-PATH?:PATH)% %PROTOCOL%" %REQ(:PATH)% %UPSTREAM_PROTOCOL% %RESPONSE_CODE% %RESPONSE_FLAGS% %RESPONSE_CODE_DETAILS% %CONNECTION_TERMINATION_DETAILS% %BYTES_RECEIVED% %BYTES_SENT% %DURATION% %REQUEST_TX_DURATION% %RESPONSE_TX_DURATION% %REQUEST_DURATION% %RESPONSE_DURATION% "%REQ(X-FORWARDED-FOR)%" "%REQ(USER-AGENT)%" "%REQ(X-REQUEST-ID)%" "%REQ(:AUTHORITY)%" "%UPSTREAM_HOST%" + [%START_TIME%] "%REQ(:METHOD)% %REQ(X-ENVOY-ORIGINAL-PATH?:PATH)% %PROTOCOL%" %REQ(:PATH)% %UPSTREAM_PROTOCOL% %RESPONSE_CODE% %RESPONSE_FLAGS% %RESPONSE_CODE_DETAILS% %CONNECTION_TERMINATION_DETAILS% %BYTES_RECEIVED% %BYTES_SENT% %DURATION% %RESP(X-ENVOY-UPSTREAM-SERVICE-TIME)% %REQUEST_TX_DURATION% %RESPONSE_TX_DURATION% %REQUEST_DURATION% %RESPONSE_DURATION% "%REQ(X-FORWARDED-FOR)%" "%REQ(USER-AGENT)%" "%REQ(X-REQUEST-ID)%" "%REQ(:AUTHORITY)%" "%UPSTREAM_HOST%"🤖 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 `@kubernetes/helm/gateway-helm-chart/values.yaml` around lines 309 - 327, The text_format template is missing the upstream service time field represented by upSvcT. Update text_format to include %RESP(X-ENVOY-UPSTREAM-SERVICE-TIME)% alongside the existing upstream timing fields, preserving the current template structure.
🤖 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.
Nitpick comments:
In `@kubernetes/helm/gateway-helm-chart/templates/gateway/gateway-config.yaml`:
- Around line 191-197: Guard the request-transformation block in the router.lua
configuration template by checking that $router.lua.request_transformation
exists before accessing its script_path field. Preserve the existing script_path
output when request_transformation is present, while allowing rendering when
router.lua contains other settings without it.
- Around line 433-437: Update the subscriptions block in the gateway
configuration template to apply a boolean default when
.Values.gateway.config.subscriptions.enable_validation is omitted. Preserve
explicitly provided values while ensuring subscriptions: {} renders valid TOML
rather than an empty value.
In `@kubernetes/helm/gateway-helm-chart/values.yaml`:
- Around line 309-327: The text_format template is missing the upstream service
time field represented by upSvcT. Update text_format to include
%RESP(X-ENVOY-UPSTREAM-SERVICE-TIME)% alongside the existing upstream timing
fields, preserving the current template structure.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b78da0df-6a18-4974-85a9-4e75c537cd97
📒 Files selected for processing (4)
kubernetes/helm/gateway-helm-chart/Chart.yamlkubernetes/helm/gateway-helm-chart/README.mdkubernetes/helm/gateway-helm-chart/templates/gateway/gateway-config.yamlkubernetes/helm/gateway-helm-chart/values.yaml
…ntroller and runtime image tags to 1.2.0-alpha2 in values.yaml
3 participants
Related to #2732