-
Notifications
You must be signed in to change notification settings - Fork 1.9k
feat(backend): add Literal parameter validation in API Server and Driver. Fixes #12602 #12607
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
feat(backend): add Literal parameter validation in API Server and Driver. Fixes #12602 #12607
Conversation
…iver Signed-off-by: Khushi Agrawal <[email protected]>
|
[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 to the Kubeflow Pipelines repo! 🎉 Thanks for opening your first PR! We're excited to have you onboard 🚀 Next steps:
Feel free to ask questions in the comments. |
|
Hi @khushiiagrawal. Thanks for your PR. I'm waiting for a kubeflow member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions 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/test-infra repository. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR implements validation for Literal runtime parameters to enforce that parameter values match a predefined set of allowed values, as specified in KEP-11385. The validation occurs at two critical points: when pipeline jobs are submitted to the API server and when the driver resolves upstream parameters at runtime.
Key Changes:
- Added
literalsfield to the proto definition for parameter specifications - Implemented validation logic in both API server and driver components
- Added comprehensive test coverage for API server validation
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| api/v2alpha1/pipeline_spec.proto | Added literals field to ComponentInputsSpec.ParameterSpec to store allowed literal values |
| api/v2alpha1/go/pipelinespec/pipeline_spec.pb.go | Regenerated Go bindings with new literals field and getter method |
| api/v2alpha1/go/cachekey/cache_key.pb.go | Updated protoc version metadata (v6.33.2) |
| backend/src/apiserver/template/v2_template.go | Extended validatePipelineJobInputs() to validate runtime parameters against literal constraints using proto.Equal() |
| backend/src/v2/driver/resolve.go | Added validateLiteralParameter() helper and integrated validation in resolveInputs() to check resolved upstream parameters |
| backend/src/apiserver/template/v2_literal_test.go | Added comprehensive test suite with 10 test cases covering valid/invalid matches for strings, numbers, booleans, and backward compatibility scenarios |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Signed-off-by: Khushi Agrawal <[email protected]>
|
/ok-to-test @alyssacgoins can you review this? |
Signed-off-by: Khushi Agrawal <[email protected]>
Signed-off-by: Khushi Agrawal <[email protected]>
|
hii @alyssacgoins , do let me know if any changes required :) |
| // The description for this input parameter of the component. | ||
| // Should not exceed 1024 characters. | ||
| Description string `protobuf:"bytes,5,opt,name=description,proto3" json:"description,omitempty"` | ||
| Description string `protobuf:"bytes,5,opt,name=description,proto3" json:"description,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like you updated this line for Description in additional to adding Literals. Can you revert this line back?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file is auto-generated by protoc from the .proto file. When I added the Literals field to pipeline_spec.proto and regenerated the Go code, the protoc compiler reformatted this line as part of the regeneration. I can't selectively revert just this line without breaking the generated code. The spacing change has no functional impact , it's just a formatting difference from the protobuf compiler version.
Thanks!
Hey @khushiiagrawal thanks for your PR - I've added a round of comments. |
Thanks @alyssacgoins for taking the time to review and for the detailed comments. I’ll go through them and make the required changes. |
|
@droctothorpe @alyssacgoins |
|
@alyssacgoins Please take a look! |
Description of your changes:
This PR implements validation for Literal runtime parameters as part of KEP-11385.
Fixes #12602
Changes Made
1. Proto Definition (api/v2alpha1/pipeline_spec.proto)
literalsfield toComponentInputsSpec.ParameterSpec2. API Server Validation (backend/src/apiserver/template/v2_template.go)
literalsfieldliteralsis non-empty, validates that the runtime value matches one of the allowed literals usingproto.Equal()3. Driver Validation (backend/src/v2/driver/resolve.go)
4. Unit Tests (backend/src/apiserver/template/v2_literal_test.go)
Backward Compatibility
literalsfield continue to work unchangedliteralsarray is treated the same as no literals (no constraint)Test Results
All 16 tests pass (14 existing + 2 new test functions with sub-tests)
Checklist: