fix: prevent nil dereference and dropped errors in RAGEngine validation - #1893
fix: prevent nil dereference and dropped errors in RAGEngine validation#1893archy-rock3t-cloud wants to merge 1 commit into
Conversation
|
Failed to generate code suggestions for PR |
214f167 to
1dbf830
Compare
GitHub Actions
Failure
GitHub Actions
Failure
GitHub Actions
Inactive
GitHub Actions
Failure
GitHub Actions
Error
There was a problem hiding this comment.
Pull request overview
This PR updates RAGEngine admission validation (both v1alpha1 and v1beta1) to avoid nil dereferences and to correctly aggregate/return embedding validation errors, along with corresponding unit test additions.
Changes:
- Add nil guards around
InferenceServicevalidation to prevent dereferencing a nil pointer. - Guard
Computevalidation and start aggregating embedding validation errors viaerrs.Also(...). - Extend validation unit tests with cases for nil
InferenceService, nilCompute, and invalid local embedding image.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| api/v1beta1/ragengine_validation.go | Adds nil guard for InferenceService and begins aggregating embedding validation errors; adds a nil check around Compute validation. |
| api/v1beta1/ragengine_validation_test.go | Adds unit tests covering nil InferenceService, nil Compute, and embedding error propagation. |
| api/v1alpha1/ragengine_validation.go | Same validation adjustments as v1beta1. |
| api/v1alpha1/ragengine_validation_test.go | Same unit test additions as v1beta1. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if w.Spec.Compute != nil { | ||
| errs = errs.Also(w.Spec.Compute.validateRAGCreate()) | ||
| } |
There was a problem hiding this comment.
Compute is treated as optional here (validation is skipped when nil), but downstream RAGEngine controller/manifests dereference Spec.Compute unconditionally (e.g., to read InstanceType and LabelSelector). This can let invalid specs pass admission and then panic at reconcile time. Consider preserving the pre-existing effective requirement by returning a missing-field error when w.Spec.Compute == nil (or at least when local embedding is used), and only then calling validateRAGCreate().
| if w.Spec.Compute != nil { | ||
| errs = errs.Also(w.Spec.Compute.validateRAGCreate()) | ||
| } |
There was a problem hiding this comment.
Compute is treated as optional here (validation is skipped when nil), but the RAGEngine controller/manifests dereference Spec.Compute unconditionally. This can let invalid specs pass admission and then panic at reconcile time. Consider returning a missing-field error when w.Spec.Compute == nil (or at least when local embedding is used) and only then calling validateRAGCreate().
| }, | ||
| }, | ||
| }, | ||
| wantErr: false, |
There was a problem hiding this comment.
This test asserts that Compute can be nil with a local embedding spec and still validate successfully. Given the controller/manifests dereference Spec.Compute (InstanceType/LabelSelector) when reconciling a RAGEngine, allowing nil here is likely to cause runtime panics. Either adjust validation to require Compute (and update this test accordingly), or update the controller/manifests to safely handle a nil Compute.
| wantErr: false, | |
| wantErr: true, | |
| errField: "Compute must be specified", |
| }, | ||
| }, | ||
| }, | ||
| wantErr: false, |
There was a problem hiding this comment.
This test asserts that Compute can be nil with a local embedding spec and still validate successfully. Given the controller/manifests dereference Spec.Compute (InstanceType/LabelSelector) when reconciling a RAGEngine, allowing nil here is likely to cause runtime panics. Either adjust validation to require Compute (and update this test accordingly), or update the controller/manifests to safely handle a nil Compute.
| wantErr: false, | |
| wantErr: true, | |
| errField: "", |
| errs = errs.Also(w.Spec.Compute.validateRAGCreate()) | ||
| if w.Spec.Compute != nil { | ||
| errs = errs.Also(w.Spec.Compute.validateRAGCreate()) | ||
| } |
There was a problem hiding this comment.
We should error on nil compute as well
| }, | ||
| }, | ||
| }, | ||
| wantErr: false, |
|
@archy-rock3t-cloud please make updates |
05984b1 to
b0eef6e
Compare
|
@bfoley13 updated |
9811b88 to
389e86d
Compare
Signed-off-by: Artem Muterko <artem@sopho.tech>
389e86d to
050037f
Compare
|
@bfoley13 friendly bump — addressed both nil-Compute checks (v1alpha1 + v1beta1 validation.go) and added the corresponding "Nil Compute" test case asserting it errors. Rebased on main. Anything else needed? |
|
This pull request has been automatically marked as stale because it has not had activity in the last 90 days. It will be closed in 14 days if no further activity occurs. Thank you for your contributions. |
Reason for Change:
Add nil guards for InferenceService and Compute, and capture previously discarded embedding validation errors in both v1alpha1 and v1beta1.
Requirements
Issue Fixed:
Notes for Reviewers: