Conversation
Add autoscaling.type toggle ("hpa" default, "keda") to switch between
HPA+DatadogMetric and KEDA ScaledObject for SQS queue-based autoscaling.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
lzboskovic1
requested review from
agromovsky,
mmalanowski and
prodm8l
as code owners
📝 WalkthroughWalkthroughAdds KEDA autoscaling support to the Kubernetes service chart, bumps chart version to 1.2.0, adds a ScaledObject template, and conditions HPA/Datadog metric rendering based on the new Changes
Sequence Diagram(s)sequenceDiagram
participant User as Developer/Helm
participant K8s as Kubernetes API
participant Chart as Chart Templates
participant KEDA as KEDA Operator
participant HPA as HPA Controller
participant Datadog as Datadog Metric Provider
User->>Chart: Render templates with values (autoscaling.type)
Chart->>K8s: Apply rendered resources
alt autoscaling.type == "keda"
Chart->>K8s: Create ScaledObject
KEDA->>K8s: Monitor triggers (SQS / CPU / Memory)
KEDA->>K8s: Scale Deployment/Rollout
else autoscaling.type != "keda"
Chart->>K8s: Create DatadogMetric (if configured)
Datadog->>HPA: Provide metric
K8s->>HPA: HPA adjusts replicas based on metrics
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 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)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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: 3
🧹 Nitpick comments (3)
charts/kubernetes-service/templates/scaled-object.yaml (2)
18-19:pollingInterval/cooldownPeriodcan be simplified using thedefaultpipeline.The inline
{{ if and ... }}...{{ else }}...{{ end }}pattern can be replaced with the idiomatic Helmdefaultfunction, which is safe even when.Values.autoscaling.kedaisnil(accessing a nil map key returns nil in Go templates, andnil | default 30yields30).♻️ Proposed simplification
- pollingInterval: {{ if and .Values.autoscaling.keda .Values.autoscaling.keda.pollingInterval }}{{ .Values.autoscaling.keda.pollingInterval }}{{ else }}30{{ end }} - cooldownPeriod: {{ if and .Values.autoscaling.keda .Values.autoscaling.keda.cooldownPeriod }}{{ .Values.autoscaling.keda.cooldownPeriod }}{{ else }}300{{ end }} + pollingInterval: {{ .Values.autoscaling.keda.pollingInterval | default 30 }} + cooldownPeriod: {{ .Values.autoscaling.keda.cooldownPeriod | default 300 }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@charts/kubernetes-service/templates/scaled-object.yaml` around lines 18 - 19, Replace the verbose conditional blocks for pollingInterval and cooldownPeriod with the Helm default pipeline: use the values .Values.autoscaling.keda.pollingInterval and .Values.autoscaling.keda.cooldownPeriod piped into default (e.g., ".Values.autoscaling.keda.pollingInterval | default 30") so the template returns the provided value when present and 30/300 otherwise; ensure you reference the same symbols pollingInterval and cooldownPeriod in the scaled-object.yaml template and keep the defaults 30 and 300 respectively.
11-15:scaleTargetRefomits explicitkind/apiVersionfor the Deployment case;defaultarguments are also swapped.Two independent observations on this block:
Missing
elsebranch: The HPA template (hpa.yamllines 17–20) explicitly emitsapiVersion: apps/v1 / kind: Deploymentwhen rollouts are disabled. The KEDA template silently relies on KEDA's own defaults (apps/v1 / Deployment). This works today but is inconsistent and fragile if KEDA ever changes its defaults. Consider adding the{{- else }}branch for symmetry.
defaultargument order:( default .Values.rollout.blueGreen.enabled false )has its arguments reversed — the canonical signature isdefault DEFAULT_VALUE INPUT_VALUE. Becausefalseis treated as empty by Helm, this always evaluates to.Values.rollout.blueGreen.enabledregardless of the literalfalse, making thefalseargument a no-op. The same pattern exists inhpa.yamlline 14 as a pre-existing issue, and behaviour happens to be equivalent here, but the intent is clearer with the correct order.♻️ Proposed fix
scaleTargetRef: - {{- if or ( default .Values.rollout.blueGreen.enabled false ) ( default .Values.rollout.canary.enabled false ) }} + {{- if or ( default false .Values.rollout.blueGreen.enabled ) ( default false .Values.rollout.canary.enabled ) }} apiVersion: argoproj.io/v1alpha1 kind: Rollout - {{- end }} + {{- else }} + apiVersion: apps/v1 + kind: Deployment + {{- end }} name: {{ include "kubernetes-service.fullname" . }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@charts/kubernetes-service/templates/scaled-object.yaml` around lines 11 - 15, The scaled-object.yaml block omits an else branch emitting explicit apiVersion/kind for a Deployment and uses Helm default() with arguments reversed; update the conditional to use correct default ordering (e.g., default false .Values.rollout.blueGreen.enabled and default false .Values.rollout.canary.enabled) and add an {{- else }} branch that emits "apiVersion: apps/v1" and "kind: Deployment" (matching the hpa.yaml behavior) so KEDA doesn't rely on implicit defaults when rollouts are disabled; locate the conditional around the Rollout kind in scaled-object.yaml to implement these changes.charts/kubernetes-service/templates/datadog-metric.yml (1)
15-16: Consider hoisting theautoscaling.typecheck outside therangeloop.The
(ne (default "hpa" $.Values.autoscaling.type) "keda")guard is re-evaluated for every SQS entry whenautoscaling.type: keda. Moving it outside the loop is a minor cleanup.♻️ Proposed refactor
-{{- range $index, $queue := .Values.autoscaling.sqs }} -{{- if and $queue.queueName $queue.targetQueueLength (ne (default "hpa" $.Values.autoscaling.type) "keda") }} +{{- if ne (default "hpa" .Values.autoscaling.type) "keda" }} +{{- range $index, $queue := .Values.autoscaling.sqs }} +{{- if and $queue.queueName $queue.targetQueueLength }} --- ... {{- end }} +{{- end }} {{- end }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@charts/kubernetes-service/templates/datadog-metric.yml` around lines 15 - 16, Hoist the autoscaling.type check out of the per-queue loop: before iterating .Values.autoscaling.sqs, evaluate (ne (default "hpa" $.Values.autoscaling.type) "keda") once and only proceed into the {{- range $index, $queue := .Values.autoscaling.sqs }} block if true; keep the existing per-queue guards for $queue.queueName and $queue.targetQueueLength but remove the repeated autoscaling.type check inside the loop so the template does not re-evaluate the same condition for every SQS entry.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@charts/kubernetes-service/templates/scaled-object.yaml`:
- Around line 44-45: The template overrides KEDA's default by setting
identityOwner: {{ default "operator" $queue.identityOwner }} which diverges from
KEDA's default of "pod"; update the charts to either revert to KEDA's default
(use "pod") or, if keeping "operator" for the IRSA operator pattern, add a clear
comment in the values (and near the identityOwner rendering in
scaled-object.yaml) explaining that the chart deliberately defaults to
"operator" and the implications for consumers who expect KEDA's default behavior
so they can opt-in by setting queue.identityOwner explicitly.
- Around line 26-46: The triggers block can render empty causing KEDA to create
a silent default CPU HPA; update
charts/kubernetes-service/templates/scaled-object.yaml to detect when no trigger
is emitted (check .Values.autoscaling.targetCPUUtilizationPercentage,
.Values.autoscaling.targetMemoryUtilizationPercentage and whether range
.Values.autoscaling.sqs yielded any entries) and fail the Helm render with a
descriptive message (using the Helm fail function) if none are present, or
alternatively add a validation comment and require at least one of those values;
reference the triggers block and the keys
.Values.autoscaling.targetCPUUtilizationPercentage,
.Values.autoscaling.targetMemoryUtilizationPercentage and
.Values.autoscaling.sqs to locate the code to change.
- Around line 39-46: The SQS KEDA trigger renders an empty mandatory queueURL
when an entry only has queueName; wrap the queueURL emission inside a
conditional so it is only rendered when present: in the template that iterates
over .Values.autoscaling.sqs (the {{- range $index, $queue :=
.Values.autoscaling.sqs }} block), guard the queueURL line with {{- if
$queue.queueURL }} queueURL: {{ $queue.queueURL }} {{- end }} (or use
with/hasKey equivalent) so no empty queueURL field is emitted for entries that
only provide queueName.
---
Nitpick comments:
In `@charts/kubernetes-service/templates/datadog-metric.yml`:
- Around line 15-16: Hoist the autoscaling.type check out of the per-queue loop:
before iterating .Values.autoscaling.sqs, evaluate (ne (default "hpa"
$.Values.autoscaling.type) "keda") once and only proceed into the {{- range
$index, $queue := .Values.autoscaling.sqs }} block if true; keep the existing
per-queue guards for $queue.queueName and $queue.targetQueueLength but remove
the repeated autoscaling.type check inside the loop so the template does not
re-evaluate the same condition for every SQS entry.
In `@charts/kubernetes-service/templates/scaled-object.yaml`:
- Around line 18-19: Replace the verbose conditional blocks for pollingInterval
and cooldownPeriod with the Helm default pipeline: use the values
.Values.autoscaling.keda.pollingInterval and
.Values.autoscaling.keda.cooldownPeriod piped into default (e.g.,
".Values.autoscaling.keda.pollingInterval | default 30") so the template returns
the provided value when present and 30/300 otherwise; ensure you reference the
same symbols pollingInterval and cooldownPeriod in the scaled-object.yaml
template and keep the defaults 30 and 300 respectively.
- Around line 11-15: The scaled-object.yaml block omits an else branch emitting
explicit apiVersion/kind for a Deployment and uses Helm default() with arguments
reversed; update the conditional to use correct default ordering (e.g., default
false .Values.rollout.blueGreen.enabled and default false
.Values.rollout.canary.enabled) and add an {{- else }} branch that emits
"apiVersion: apps/v1" and "kind: Deployment" (matching the hpa.yaml behavior) so
KEDA doesn't rely on implicit defaults when rollouts are disabled; locate the
conditional around the Rollout kind in scaled-object.yaml to implement these
changes.
| triggers: | ||
| {{- if .Values.autoscaling.targetCPUUtilizationPercentage }} | ||
| - type: cpu | ||
| metricType: Utilization | ||
| metadata: | ||
| value: {{ .Values.autoscaling.targetCPUUtilizationPercentage | quote }} | ||
| {{- end }} | ||
| {{- if .Values.autoscaling.targetMemoryUtilizationPercentage }} | ||
| - type: memory | ||
| metricType: Utilization | ||
| metadata: | ||
| value: {{ .Values.autoscaling.targetMemoryUtilizationPercentage | quote }} | ||
| {{- end }} | ||
| {{- range $index, $queue := .Values.autoscaling.sqs }} | ||
| - type: aws-sqs-queue | ||
| metadata: | ||
| queueURL: {{ $queue.queueURL }} | ||
| queueLength: {{ $queue.targetQueueLength | quote }} | ||
| awsRegion: {{ default "us-east-2" $queue.awsRegion }} | ||
| identityOwner: {{ default "operator" $queue.identityOwner }} | ||
| {{- end }} |
There was a problem hiding this comment.
Empty triggers: block produces a silent default-CPU HPA.
If none of the three trigger sections (CPU, memory, SQS) emit any entries, the rendered manifest becomes triggers: null (or an empty block). When triggers is set with an empty array in a ScaledObject, KEDA doesn't validate this empty value and will create a default HPA with a default 80% averageUtilization CPU resource. This is unlikely in practice because targetCPUUtilizationPercentage: 80 is set in values.yaml by default, but a service that explicitly zeroes it out and has no SQS queues would hit this silently.
Consider adding a validation comment or a Helm fail guard if no trigger is resolvable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@charts/kubernetes-service/templates/scaled-object.yaml` around lines 26 - 46,
The triggers block can render empty causing KEDA to create a silent default CPU
HPA; update charts/kubernetes-service/templates/scaled-object.yaml to detect
when no trigger is emitted (check
.Values.autoscaling.targetCPUUtilizationPercentage,
.Values.autoscaling.targetMemoryUtilizationPercentage and whether range
.Values.autoscaling.sqs yielded any entries) and fail the Helm render with a
descriptive message (using the Helm fail function) if none are present, or
alternatively add a validation comment and require at least one of those values;
reference the triggers block and the keys
.Values.autoscaling.targetCPUUtilizationPercentage,
.Values.autoscaling.targetMemoryUtilizationPercentage and
.Values.autoscaling.sqs to locate the code to change.
Address CodeRabbit review: skip SQS trigger when queueURL is missing, add comment explaining operator identity default for IRSA. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
charts/kubernetes-service/templates/scaled-object.yaml (2)
11-11: Non-idiomaticdefaultargument order.The Helm
defaultfunction signature isdefault DEFAULT_VALUE GIVEN_VALUE, so the intent "usefalsewhen.Values.rollout.blueGreen.enabledis not set" should be written asdefault false .Values.rollout.blueGreen.enabled.The current form
default .Values.rollout.blueGreen.enabled falsepasses the literalfalseas the value-to-test — sincefalseis always treated as empty by Sprig, this unconditionally returns.Values.rollout.blueGreen.enabled. It produces the same boolean result at runtime (nil and false are both falsy in{{- if }}), but the semantics are inverted and misleading. Same applies to the canary sub-expression.♻️ Proposed fix
- {{- if or ( default .Values.rollout.blueGreen.enabled false ) ( default .Values.rollout.canary.enabled false ) }} + {{- if or (default false .Values.rollout.blueGreen.enabled) (default false .Values.rollout.canary.enabled) }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@charts/kubernetes-service/templates/scaled-object.yaml` at line 11, The Helm template uses the `default` function with arguments reversed in the expression `if or ( default .Values.rollout.blueGreen.enabled false ) ( default .Values.rollout.canary.enabled false )`; update both uses so the default value comes first (e.g., `default false .Values.rollout.blueGreen.enabled` and `default false .Values.rollout.canary.enabled`) so the semantics match Sprig's `default DEFAULT GIVEN` signature and the `if` behaves as intended.
18-19: Simplify nestedkedafield access withdig.The inline
if and ... else ... endpattern on both lines is verbose. Helm/Sprig'sdigfunction safely traverses nested maps and returns a default when any key is absent.♻️ Proposed refactor
- pollingInterval: {{ if and .Values.autoscaling.keda .Values.autoscaling.keda.pollingInterval }}{{ .Values.autoscaling.keda.pollingInterval }}{{ else }}30{{ end }} - cooldownPeriod: {{ if and .Values.autoscaling.keda .Values.autoscaling.keda.cooldownPeriod }}{{ .Values.autoscaling.keda.cooldownPeriod }}{{ else }}300{{ end }} + pollingInterval: {{ dig "keda" "pollingInterval" 30 .Values.autoscaling }} + cooldownPeriod: {{ dig "keda" "cooldownPeriod" 300 .Values.autoscaling }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@charts/kubernetes-service/templates/scaled-object.yaml` around lines 18 - 19, Replace the verbose conditional expressions that check .Values.autoscaling.keda.pollingInterval and .Values.autoscaling.keda.cooldownPeriod with Sprig's dig to safely fetch nested values with defaults: use dig .Values "autoscaling" "keda" "pollingInterval" with default 30 for pollingInterval and dig .Values "autoscaling" "keda" "cooldownPeriod" with default 300 for cooldownPeriod; update the template lines that currently set pollingInterval and cooldownPeriod so they call dig and fall back to the numeric defaults while preserving the current output format.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@charts/kubernetes-service/templates/scaled-object.yaml`:
- Line 43: The emitted queueURL value is unquoted while queueLength uses |
quote; update the template key queueURL to output a quoted string by applying |
quote to the variable (replace {{ $queue.queueURL }} with {{ $queue.queueURL |
quote }} or otherwise ensure it is wrapped as a quoted scalar) so the YAML stays
consistent and safe against special-character endpoints.
---
Duplicate comments:
In `@charts/kubernetes-service/templates/scaled-object.yaml`:
- Around line 26-49: The rendered "triggers:" block can be empty when
.Values.autoscaling.targetCPUUtilizationPercentage,
.Values.autoscaling.targetMemoryUtilizationPercentage, and all
.Values.autoscaling.sqs[*].queueURL are unset; wrap the entire triggers: section
with a guard that checks for at least one configured trigger (e.g. an if that
tests (or (gt .Values.autoscaling.targetCPUUtilizationPercentage 0) (gt
.Values.autoscaling.targetMemoryUtilizationPercentage 0) (any non-empty
.Values.autoscaling.sqs[*].queueURL)) ) and only emit "triggers:" and its
entries when true, otherwise call fail (or omit the ScaledObject) to prevent
emitting an empty key; update scaled-object.yaml around the triggers block and
reference .Values.autoscaling.sqs,
.Values.autoscaling.targetCPUUtilizationPercentage, and
.Values.autoscaling.targetMemoryUtilizationPercentage when implementing the
check.
---
Nitpick comments:
In `@charts/kubernetes-service/templates/scaled-object.yaml`:
- Line 11: The Helm template uses the `default` function with arguments reversed
in the expression `if or ( default .Values.rollout.blueGreen.enabled false ) (
default .Values.rollout.canary.enabled false )`; update both uses so the default
value comes first (e.g., `default false .Values.rollout.blueGreen.enabled` and
`default false .Values.rollout.canary.enabled`) so the semantics match Sprig's
`default DEFAULT GIVEN` signature and the `if` behaves as intended.
- Around line 18-19: Replace the verbose conditional expressions that check
.Values.autoscaling.keda.pollingInterval and
.Values.autoscaling.keda.cooldownPeriod with Sprig's dig to safely fetch nested
values with defaults: use dig .Values "autoscaling" "keda" "pollingInterval"
with default 30 for pollingInterval and dig .Values "autoscaling" "keda"
"cooldownPeriod" with default 300 for cooldownPeriod; update the template lines
that currently set pollingInterval and cooldownPeriod so they call dig and fall
back to the numeric defaults while preserving the current output format.
| {{- if $queue.queueURL }} | ||
| - type: aws-sqs-queue | ||
| metadata: | ||
| queueURL: {{ $queue.queueURL }} |
There was a problem hiding this comment.
queueURL should be quoted for consistency and safety.
queueLength on line 44 uses | quote, but queueURL is emitted bare. While standard SQS URLs are valid unquoted YAML scalars today, quoting dynamic string values is safer against edge cases (e.g., custom endpoint URLs with characters that can confuse the YAML parser) and is consistent with the rest of the metadata block.
♻️ Proposed fix
- queueURL: {{ $queue.queueURL }}
+ queueURL: {{ $queue.queueURL | quote }}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| queueURL: {{ $queue.queueURL }} | |
| queueURL: {{ $queue.queueURL | quote }} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@charts/kubernetes-service/templates/scaled-object.yaml` at line 43, The
emitted queueURL value is unquoted while queueLength uses | quote; update the
template key queueURL to output a quoted string by applying | quote to the
variable (replace {{ $queue.queueURL }} with {{ $queue.queueURL | quote }} or
otherwise ensure it is wrapped as a quoted scalar) so the YAML stays consistent
and safe against special-character endpoints.
2 participants
Summary
autoscaling.typetoggle (hpadefault,keda) to kubernetes-service Helm charttemplates/scaled-object.yamlrenders KEDA ScaledObject withaws-sqs-queuetriggerstype: kedatype: hpawith no changes neededTest plan
autoscaling.type: kedaand verify ScaledObject is createdtype: hpa(or unset) continue to work unchanged🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Chores