knative / knative/func

Deployers duplicate validation because Client.Deploy does not call Function.Validate()

Open
#4,055 0 comments 0 reactions 0 assignees View on GitHub
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.