fix: validate modelroute target entries#1061
Conversation
Signed-off-by: pm-ju <pmdevops29@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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces validation for TargetModels in both the datastore logic and the admission webhook, ensuring that target lists are not empty, entries are not nil, and ModelServerName is specified. Corresponding unit tests were added to verify these checks. Feedback suggests extending the webhook validation to enforce weight consistency across all targets in a rule, matching the existing runtime behavior to avoid debugging difficulties.
| for j, target := range rule.TargetModels { | ||
| targetField := ruleField.Child("targetModels").Index(j) | ||
| if target == nil { | ||
| allErrs = append(allErrs, field.Invalid(targetField, target, "target model must not be nil")) | ||
| continue | ||
| } | ||
| if target.ModelServerName == "" { | ||
| allErrs = append(allErrs, field.Required(targetField.Child("modelServerName"), "modelServerName must be specified")) | ||
| } | ||
| } |
There was a problem hiding this comment.
The validating webhook should also enforce consistency in the weight field across all targetModels in a rule. Currently, the datastore runtime (in toWeightedSlice) rejects rules where some targets have weights and others don't, but the webhook allows them. This leads to valid-looking objects being ignored at runtime, which is difficult to debug. Please add a check to ensure that either all targetModels have a weight specified or none of them do.
| if len(targets) == 0 { | ||
| return nil, fmt.Errorf("no target models specified in rule") | ||
| } | ||
| if targets[0] == nil { | ||
| return nil, fmt.Errorf("targetModel[0] must not be nil") | ||
| } |
There was a problem hiding this comment.
// +kubebuilder:validation:MinItems=1
// +kubebuilder:validation:MaxItems=16
TargetModels []*TargetModel `json:"targetModels"`
we already get this.
There was a problem hiding this comment.
You’re right, the empty targetModels case is already covered by MinItems=1, and the runtime guard was redundant.
I narrowed the PR to only validate empty targetModels[].modelServerName in the webhook, since required does not necessarily reject an explicitly empty string.
Signed-off-by: pm-ju <pmdevops29@gmail.com>
| } | ||
| for j, target := range rule.TargetModels { | ||
| targetField := ruleField.Child("targetModels").Index(j) | ||
| if target != nil && target.ModelServerName == "" { |
There was a problem hiding this comment.
Ptal // +kubebuilder:validation:required, so this is not needed
/kind bug
ModelRoute validation only checked that each rule had at least one targetModels entry. It did not reject nil target entries or empty modelServerName values.
That can admit invalid routes and also lets bad objects reach router runtime selection, where targetModels are dereferenced while building weighted routing choices.
Fixes #1060
This change:
Verification:
git diff --check
git diff --cached --check