Deployers duplicate validation because Client.Deploy does not call Function.Validate()
- Dominant language
- Go
- Stars
- 365
- Forks
- 223
- Avg merge
- 2d 3h
- Merged PRs (30d)
- 25
Description
### Summary
Each deployer's `Deploy` method (most visibly `pkg/keda/deployer.go`) re-implements a large block of validation that `Function.Validate()` already performs (`ValidateScale`, `ValidateKafkaSecurity`, scale bounds, trigger type/dup/combination checks). Ideally a deployer is a minimal executor and only validates what is genuinely deploy-specific (k8s resource-name limits, secret/volume path resolution).
### Why the duplication exists today
`Client.Deploy` (`pkg/functions/client.go:847`) does **not** call `Function.Validate()` before invoking `c.deployer.Deploy(...)` — it only checks `Built`, `Name`, `ValidateExpose`, and `ValidateSwitch`. So on the library path (`client.Deploy(ctx, f)` with an in-memory function), the deployer's own preflight is the *only* validation that runs. Some callers invoke `deployer.Deploy` directly, bypassing `Client.Deploy` entirely.
As a result, the keda deployer's guards are currently load-bearing: removing them would let the library path silently create partial/broken resources (a raw Deployment with no scaler, a ScaledObject that can't connect to any broker, a resource name that overflows the 63-char DNS limit and fails server-side).
Only the CLI (`cmd/deploy.go:316`) and Tekton (`pkg/pipelines/tekton/pipelines_provider.go:201`) paths call `Function.Validate()` up front — so for those, the deployer re-validation truly is duplication.
### Proposed fix
Introduce a single validation choke point: call `Function.Validate()` inside `Client.Deploy` (once), then thin **all** deployers (knative / raw / keda) down to the checks that `Function.Validate` does not cover:
- k8s resource-name / DNS-label length limits (`validateBridgeName`, `validateKafkaResourceNames`)
- secret / TLS-path resolution against `f.Run.Volumes` (`validateKafkaTLSPaths`)
- nil-client guards
This removes the duplication *and* closes the latent gap where library consumers of `Client.Deploy` skip validation entirely.
### Scope / notes
- Cross-deployer refactor; deserves its own PR rather than riding along in the KEDA scaling PR (#4051).
- Watch the direct-`deployer.Deploy` callers (e.g. `pkg/deployer/testing/integration_test_helper.go`) — they rely on the deployer validating today, so they'd need to validate up front (or keep going through `Client.Deploy`).
- Flagged by @gauron99 in the #4051 review (keda deployer should be a minimal executor).
Contributor guide
Research direction
Start at Client.Deploy in pkg/functions/client.go:847 and compare the validation in pkg/keda/deployer.go with the knative and raw deployers. Inspect the direct deployer callers, including pkg/deployer/testing/integration_test_helper.go, before changing the validation boundary. Done means Client.Deploy validates once, deployers retain only deployer-specific checks, and direct callers still validate before deployment.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- go, kubernetes
- Domain
- devops, infrastructure
- Issue type
- Refactor
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Active
- Clarity
- Mostly clear
- Newbie friendliness
- 45/100