fix(autoscaler): validate replica range for all target types - #1678
fix(autoscaler): validate replica range for all target types#1678vanshika2720 wants to merge 2 commits into
Conversation
Disaggregated targets already reject minReplicas > maxReplicas, but Homogeneous and Heterogeneous targets had no equivalent check, letting invalid ranges reach downstream scaling logic. For Heterogeneous targets this allows a backend's replica count to bypass its own configured maxReplicas cap. Add the same minReplicas <= maxReplicas guard to the Homogeneous and Heterogeneous branches of validateTarget, mirroring the existing Disaggregated check, and add regression tests for min<max, min==max, and min>max across all three target types. Signed-off-by: Vanshika <pahalvanshikaa@gmail.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
@YaoZengzeng PTAL! |
LiZhenCheng9527
left a comment
LiZhenCheng9527
left a comment
There was a problem hiding this comment.
These two tests can be combined into a table-driven table.
| minGreaterThanMax.Spec.HomogeneousTarget.MaxReplicas = 2 | ||
| allowed, msg = validator.validateAutoscalingPolicy(minGreaterThanMax) | ||
| assert.False(t, allowed) | ||
| assert.Contains(t, msg, "minReplicas must be <= maxReplicas") |
There was a problem hiding this comment.
Only correcting error messages? It is recommended to also check the field paths.
Combine the Homogeneous and Heterogeneous minReplicas/maxReplicas regression tests into one table-driven test, per review feedback, and call validateTarget directly to assert on the returned field.ErrorList instead of just the flattened message string. validateTarget already builds proper field.Path values (spec.homogeneousTarget.minReplicas, spec.heterogeneousTarget.params[i].minReplicas), so this only needed a test change, not a validator change. Add the same field-path assertion to the existing Disaggregated invalid-range case for consistency, since validateDisaggregatedTarget already returns the equivalent structured path (spec.disaggregatedTarget.roles[name].minReplicas) and had the same gap. Signed-off-by: Vanshika <pahalvanshikaa@gmail.com>
|
New changes are detected. LGTM label has been removed. |
|
@LiZhenCheng9527 I combined the Homogeneous and Heterogeneous replica-range cases into a table-driven test and added explicit field.ErrorList/field-path assertions instead of only checking the error message. I also added the corresponding field-path assertion for the existing Disaggregated case for consistency |
| ¶m.Target.TargetRef, | ||
| specPath.Child("heterogeneousTarget").Child("params").Index(idx).Child("target").Child("targetRef"))...) | ||
| paramPath.Child("target").Child("targetRef"))...) | ||
| if param.MinReplicas > param.MaxReplicas { |
There was a problem hiding this comment.
The validation itself is correct and consistent with the existing Disaggregated check.
Non-blocking (defense-in-depth): this fix closes the admission front door, but the downstream code path you identified still trusts the min <= max invariant. In NewOptimizerMeta, a param with MinReplicas > MaxReplicas produces replicas := MaxReplicas - MinReplicas <= 0 and is skipped from scalingOrder, so RestoreReplicasOfEachBackend leaves that backend pinned at MinReplicas (above MaxReplicas). A webhook can be bypassed (not installed, failurePolicy: Ignore, or objects persisted before this validation existed), and such stored policies would still hit the exact bypass this PR describes. Consider clamping in RestoreReplicasOfEachBackend/NewOptimizerMeta as well, or opening a follow-up, so the mitigation isn't limited to newly-admitted objects.
5 participants
What this PR does / why we need it
The autoscaler currently validates
minReplicas <= maxReplicasonly for Disaggregated targets. Homogeneous and Heterogeneous targets can accept an invalid replica range whereminReplicas > maxReplicas.This change applies the same validation to Homogeneous and Heterogeneous targets, ensuring all supported target types reject invalid replica ranges consistently.
For Heterogeneous targets, the invalid range can propagate into the scaling logic and allow a backend's replica count to bypass its configured
maxReplicaslimit.The change also adds regression tests covering
minReplicas < maxReplicas,minReplicas == maxReplicas, andminReplicas > maxReplicasfor the affected target types.Which issue(s) this PR fixes
Fixes #1668
Bug evidence (required for bug-related PRs)
The production path is:
AutoscalingPolicy validation →
validateTarget()→ target-specific validation → autoscaling optimization → replica restoration.For Homogeneous and Heterogeneous targets,
validateTarget()previously did not enforceminReplicas <= maxReplicas. A configuration such as:was therefore accepted.
For Heterogeneous targets, this invalid range can reach
RestoreReplicasOfEachBackend, where a backend's replica count is initialized fromMinReplicas. When the backend is not subsequently included in the scaling order, the configuredMaxReplicasis not re-applied, allowing the resulting replica count to exceed the configured maximum.The fix rejects the invalid configuration during validation, before it reaches the downstream scaling logic.
Special notes for your reviewer
The existing Disaggregated validation is reused as the model for Homogeneous and Heterogeneous targets. No API or CRD changes are required.
Regression tests cover valid and invalid replica ranges for the target types.
Does this PR introduce a user-facing change?
No. Existing valid configurations continue to work unchanged. Invalid configurations with
minReplicas > maxReplicasare now rejected consistently across all target types.