Validation webhook implementation #1682
Conversation
* implementation of dummy validation webhook --------- Co-authored-by: Patryk Wasielewski <pwasiele@splunk.com>
Implement Validation Webhook logic + unit tests + mux server for webhook --------- Co-authored-by: Patryk Wasielewski <pwasiele@splunk.com>
|
CLA Assistant Lite bot: I have read the CLA Document and I hereby sign the CLA 0 out of 2 committers have signed the CLA. |
Pull Request Test Coverage Report for Build 21636244635Details
💛 - Coveralls |
|
|
||
| warnings := GetClusterManagerWarningsOnCreate(obj) | ||
| if len(warnings) != 0 { | ||
| t.Errorf("GetClusterManagerWarningsOnCreate() returned %d warnings, expected 0", len(warnings)) |
There was a problem hiding this comment.
Why don't we just use assertions?
There was a problem hiding this comment.
It applies to all tests
There was a problem hiding this comment.
I agree with Kasia, we already import testify.
|
I have read the Code of Conduct and I hereby accept the Terms |
kubabuczak
left a comment
kubabuczak
left a comment
There was a problem hiding this comment.
In current code there are 26 validation functions (search func validate across codebase) - shouldn't we replace / move them to new validation package?
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| errs := ValidateClusterManagerCreate(tt.obj) |
There was a problem hiding this comment.
These validation tests are good candidates for t.Parallel() since they're stateless - each test creates its own objects and calls pure validation functions with no shared mutable state.
However, I'd suggest not adding it for now because:
Minimal benefit - These tests are already fast (~0.7s for all validation tests). Parallelization overhead might not provide meaningful speedup.
Consistency - The rest of the codebase doesn't use t.Parallel() in table-driven tests. Adding it here would create inconsistency.
Maintenance overhead - Requires the tt := tt capture pattern which can be forgotten and cause subtle bugs:
for _, tt := range tests {
tt := tt // easy to forget this line
t.Run(tt.name, func(t *testing.T) {
t.Parallel()
// ...
})
}
My reecommendation: Keep as-is for now. Consider adding t.Parallel() as a separate refactoring effort across all tests if test execution time becomes a concern.
| ReadTimeout: 10 * time.Second, | ||
| WriteTimeout: 10 * time.Second, |
There was a problem hiding this comment.
Hmm.. maybe we should expose it as some config/env variable? I could imagine that slow k8s API server may cause timeouts. On the other hand - 10 seconds should be sufficient.
| } | ||
| defer r.Body.Close() | ||
|
|
||
| // Decode AdmissionReview |
There was a problem hiding this comment.
[nit] in general I think that adding comments that describe obvious thing in code should be avoided. We should add them to explain "why" behind non-obvious code.
4 participants
Description
Implements an opt-in validation webhook for Splunk Enterprise CRDs that validates spec fields on CREATE and UPDATE operations, providing immediate feedback when invalid configurations are submitted.
The webhook is disabled by default and can be enabled by setting ENABLE_WEBHOOKS=true or using the config/default-with-webhook kustomize overlay.
Key Changes
New Validation Package (
pkg/splunk/enterprise/validation/)Validated Fields
spec.replicasspec.imagePullPolicyAlways,Never, orIfNotPresentspec.livenessInitialDelaySecondsspec.readinessInitialDelaySecondsspec.etcVolumeStorageConfig.storageCapacity^[0-9]+Gi$spec.varVolumeStorageConfig.storageCapacity^[0-9]+Gi$spec.smartstorespec.appRepoWebhook Infrastructure
config/webhook/- Service and ValidatingWebhookConfigurationconfig/certmanager/- TLS certificate via cert-managerconfig/default-with-webhook/- Opt-in kustomize overlayDocumentation
Testing and Verification
Comprehensive unit tests (~95% coverage)
Server and HTTP handler tests
Related Issues
N/A
PR Checklist