feat: template Validation and Defaults in Admission Webhooks #86
Conversation
earaghbidikashani
changed the title
earaghbidikashani
changed the title
There was a problem hiding this comment.
Hi @earaghbidikashani. Thank you for your work. Please find comments and suggestions below. P0s are a must to fix to merge this IMO.
P0:
- GPU limits validation
- Move inline YAMLs to files (maintainability)
- Multiple Template get calls
- Fix ApplyTemplateDefaults re PrimaryStorage
P1:
- GPU test coverage
- Improve validation error messages
| } | ||
|
|
||
| // validateResourceBounds checks if resources are within bounds | ||
| func (tv *TemplateValidator) validateResourceBounds(resources corev1.ResourceRequirements, bounds *workspacev1alpha1.ResourceBounds) []controller.TemplateViolation { |
There was a problem hiding this comment.
Missing GPU bounds validation / this function does not check GPU bounds. We also need to add E2E test for it or track the need in the issue.
Nit: for extra cleanliness separate validation logic for CPU, GPU etc can be broken out into separate helper functions but it's also always a bit of a tradeoff to this so up to you
There was a problem hiding this comment.
I brought this logic over from template resolver here: https://github.com/jupyter-infra/jupyter-k8s/blob/main/internal/controller/template_resolver.go#L227
You already created an issue tracking this change, the purpose of this PR is to bring the existing validation logic from operator to the webhooks. We'll remove the operator validation logic entirely (I didn't do it in this PR because env var is only in resolved template and not in workspace itself, we need to add env var in workspace and remove the dependency to resolved template entirely)
There was a problem hiding this comment.
Resolver seems to have some GPU bounds enforcement
| } | ||
|
|
||
| // Only validate storage if it changed | ||
| if !storageEqual(oldWorkspace.Spec.Storage, newWorkspace.Spec.Storage) && |
There was a problem hiding this comment.
can we scope this specifically to storage size? this helps with the equality check confusion below as well
There was a problem hiding this comment.
what about mount path? should it be immutable then?
| // Validate templateRef immutability | ||
| oldTemplateRef := fetchTemplateRef(oldWorkspace) | ||
| newTemplateRef := fetchTemplateRef(newWorkspace) | ||
| if oldTemplateRef != "" && oldTemplateRef != newTemplateRef && !isAdmin { |
There was a problem hiding this comment.
question: do you think we should allow even an admin to change the template? as opposed to deleting the Workspace?
There was a problem hiding this comment.
Currently this is following the CEL validation rule in the workspace - making the template reference immutable.
Are you suggesting that template should be immutable and template reference be mutable?
5 participants
This PR moves template validation and default application from the controller to admission webhooks, providing early validation and consistent template enforcement at admission time.
Core Implementation
• Created TemplateValidator (internal/webhook/v1alpha1/template_validator.go)
• Validates workspace template constraints during admission
• Applies template defaults to workspace specs
• Handles both create and update operations with differential validation
Validation Features
• Template Constraint Validation:
• Image allowlist enforcement
• Resource bounds checking (CPU, memory, GPU)
• Storage size limits
• Template reference validation
• Comprehensive Default Application:
• Default image, resources, and storage from templates
• Container configuration, node selectors, affinity, tolerations
• Ownership type defaults
• Template tracking labels
Webhook Integration
• Updated Workspace Webhooks:
• Integrated template validator into existing webhook infrastructure
• Maintains existing ownership and immutability validations
• Proper error handling and user feedback
Testing Updates
• E2E Test Modernization:
• Updated tests to expect webhook rejection instead of controller validation
• Added dedicated webhook validation test suite
• Tests for template defaults application and multi-violation scenarios
Behavior Changes
• Invalid workspaces are rejected at admission time
• Template defaults applied immediately during creation
• Fast validation feedback to users
• No invalid resources stored in etcd
Testing Results
The webhook validation is working correctly, as demonstrated by these test results:
Opened #87 to follow up with removing the operator validation - need to confirm on env vars