Refactor(hpa): extract common metric target validation logic #132405
Conversation
|
This issue is currently awaiting triage. If a SIG or subproject determines this is a relevant issue, they will accept it by applying the The Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: googs1025 The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/sig autoscaling |
|
/assign |
| @@ -327,10 +327,7 @@ func validateObjectSource(src *autoscaling.ObjectMetricSource, fldPath *field.Pa | |||
| allErrs = append(allErrs, ValidateCrossVersionObjectReference(src.DescribedObject, fldPath.Child("describedObject"))...) | |||
| allErrs = append(allErrs, validateMetricIdentifier(src.Metric, fldPath.Child("metric"))...) | |||
| allErrs = append(allErrs, validateMetricTarget(src.Target, fldPath.Child("target"))...) | |||
|
|
|||
| if src.Target.Value == nil && src.Target.AverageValue == nil { | |||
There was a problem hiding this comment.
If I understand correctly, it seems not to just check if both are not empty. Should we still check if both exist at the same time?
There was a problem hiding this comment.
Not sure if I understand your question but we need to check that exactly one of the values must be set. So first we check that at least one value is set and then we need to ensure both values aren't set simultaneously.
|
@adrianmoisey @omerap12 can you help this? 😄 |
|
|
||
| if target.Value != nil && target.AverageValue != nil { | ||
| allErrs = append(allErrs, field.Forbidden(fldPath.Child("value"), | ||
| "must not set both a target value or averageValue")) |
There was a problem hiding this comment.
Typo, how about something like: "must not set both a target value and averageValue"
"or" is grammatically incorrect when describing what cannot be set together. It should be "and" since we're referring to the prohibition of setting both fields simultaneously.
| if target.AverageUtilization != nil && target.AverageValue != nil { | ||
| allErrs = append(allErrs, field.Forbidden( | ||
| fldPath.Child("averageValue"), | ||
| "must not set both a target raw value and a target utilization", |
| @@ -327,10 +327,7 @@ func validateObjectSource(src *autoscaling.ObjectMetricSource, fldPath *field.Pa | |||
| allErrs = append(allErrs, ValidateCrossVersionObjectReference(src.DescribedObject, fldPath.Child("describedObject"))...) | |||
| allErrs = append(allErrs, validateMetricIdentifier(src.Metric, fldPath.Child("metric"))...) | |||
| allErrs = append(allErrs, validateMetricTarget(src.Target, fldPath.Child("target"))...) | |||
|
|
|||
| if src.Target.Value == nil && src.Target.AverageValue == nil { | |||
There was a problem hiding this comment.
Not sure if I understand your question but we need to check that exactly one of the values must be set. So first we check that at least one value is set and then we need to ensure both values aren't set simultaneously.
| if src.Target.Value == nil && src.Target.AverageValue == nil { | ||
| allErrs = append(allErrs, field.Required(fldPath.Child("target").Child("averageValue"), "must set either a target value or averageValue")) | ||
| } | ||
| allErrs = append(allErrs, validateValueOrAverageValue(src.Target, fldPath.Child("target"))...) |
There was a problem hiding this comment.
This will change behaviour for users. If they have (incorrect) HPAs that define both, they used to apply successfully and now they won't after this change. (https://www.hyrumslaw.com).
While I like that this is being added, I'm never sure how these situations should be handled to get this into k/k.
May be @soltysh can advise?
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
Previously, similar checks for mutually exclusive fields such as
Value/AverageValueandAverageUtilization/AverageValuewere duplicated across multiple validation functions likevalidateExternalSource,validateResourceSource, etc. This change consolidates that logic into shared validators to improve code maintainability and readability.Which issue(s) this PR is related to:
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: