fix: reject whitespace in webhook image validation - #1509
Conversation
|
[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 |
|
Welcome @safiya2610! It looks like this is your first PR to volcano-sh/kthena 🎉 |
|
/kind bug |
kube-gopher
left a comment
kube-gopher
left a comment
There was a problem hiding this comment.
git commit -s . please add -s
while squashing all commit in one, I will do. |
ddf9abb to
a9f4c32
Compare
| if strings.Contains(image, " ") { | ||
| return fmt.Errorf("image cannot contain spaces") | ||
| if strings.IndexFunc(image, unicode.IsSpace) != -1 { | ||
| return fmt.Errorf("image cannot contain whitespace") |
There was a problem hiding this comment.
I have added a comprehensive test suite (TestValidateImageField) in pkg/model-booster-controller/webhook/model_validator_test.go in my latest commit. It covers all the edge cases, including images with spaces, tabs, newlines, empty strings, as well as valid inputs.
| return "" | ||
| } | ||
| s := strings.Split(path, URIPrefixSeparator)[1] | ||
| _, s, _ := strings.Cut(path, URIPrefixSeparator) |
There was a problem hiding this comment.
This looks to hacky to me, we should reject invalid value
There was a problem hiding this comment.
I have completely removed the strings.Cut / strings.Split logic and replaced it with Go's standard net/url.ParseRequestURI() in both GetCachePath and cacheVolumeMountPath. This natively parses the URI, formally rejects any malformed/invalid values (returning an empty string), and safely handles nested URIs without us having to write custom string chopping logic.
Also all test suites have been verified and pass successfully
c5cb042 to
9ecf537
Compare
|
Fix the failed tests. |
doing... |
e64fe43 to
35a00ab
Compare
Signed-off-by: Safiya <147792763+safiya2610@users.noreply.github.com>
be625b6 to
91d045e
Compare
Fixed, Please review the PR. |
5 participants
This PR addresses two separate issues:
Cache URI Parsing: When users provided cache URIs that contained multiple :// separators (e.g., pvc://my-cache-path/http://some-model), the parsing logic using strings.Split would improperly truncate the path.
Webhook Image Validation: The existing webhook validation for container images only rejected exact space characters (" "), but allowed other whitespace characters like tabs or newlines to slip through, which later failed Kubernetes' strict container image validation.
Solution
Cache Paths: Refactored GetCachePath in pkg/model-booster-controller/convert/model_serving.go to use strings.Cut instead of strings.Split. This ensures that only the very first :// is treated as the URI prefix separator, preserving the rest of the path intact.
Image Validation: Updated validateImageField in pkg/model-booster-controller/webhook/model_validator.go to use strings.ContainsAny(image, " \t\r\n") to reliably reject all forms of whitespace.
Tests: Added a new table-driven test case in model_serving_test.go to verify cache paths containing multiple :// separators are no longer truncated.
Testing Done
go test ./pkg/model-booster-controller/convert - Pass
go test ./pkg/model-booster-controller/webhook - Pass
Fixes #1026