feat: functions exposed via ocp-route - keda + raw deployers - #3973
feat: functions exposed via ocp-route - keda + raw deployers#3973gauron99 wants to merge 2 commits into
Conversation
|
@gauron99: The label(s) DetailsIn response to this:
Instructions 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-sigs/prow repository. |
|
Skipping CI for Draft Pull Request. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: gauron99 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
There was a problem hiding this comment.
Pull request overview
This PR adds configurable external exposure for the raw and keda deployers via a new deploy.expose field / --expose flag, with OpenShift Route support and improved URL reporting across deploy, list, and describe flows.
Changes:
- Introduces
deploy.expose(route|none|"") with validation, CLI flag + completion, schema/docs updates, and new CLI error type for invalid values. - Implements OpenShift Route lifecycle management for the raw deployer (Route creation/removal + admission wait + recording hostname on the Service for later reads).
- Updates the keda deployer to manage its own Route to the interceptor (and keeps tests/e2e/integration helpers deterministic by opting out with
expose:none).
Reviewed changes
Copilot reviewed 37 out of 38 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| schema/func_yaml-schema.json | Adds deploy.expose to func.yaml JSON schema. |
| pkg/remover/testing/integration_test_helper.go | Forces integration test deploys to be cluster-local (Expose: "none"). |
| pkg/pipelines/tekton/pipelines_provider.go | Aligns deploy status messaging (neutral “at URL”). |
| pkg/mock/deployer.go | Sets a successful mock deploy status (now always Deployed). |
| pkg/lister/testing/integration_test_helper.go | Forces integration test deploys to be cluster-local (Expose: "none"). |
| pkg/knative/deployer.go | Adjusts verbose message wording (removes “exposed”). |
| pkg/keda/route.go | Adds OpenShift Route generation/ensure/remove helpers for the keda interceptor. |
| pkg/keda/route_test.go | Unit tests for keda interceptor Route helpers and URL selection. |
| pkg/keda/remover.go | Removes keda interceptor Route during removal on OpenShift. |
| pkg/keda/remover_test.go | Tests Route-removal ordering and nil-dynamic-client behavior. |
| pkg/keda/lister.go | Prefers external Route URL when present (by deterministic Route name). |
| pkg/keda/describer.go | Reports URLs including external Route (when admitted) + internal bridge URLs. |
| pkg/keda/deployer.go | Adds deploy.expose handling and Route lifecycle for keda (interceptor Route). |
| pkg/keda/deployer_test.go | Tests interceptor namespace resolution and bridge ExternalName. |
| pkg/k8s/wait.go | Generalizes WaitForServiceRemoved to kubernetes.Interface. |
| pkg/k8s/route.go | Adds OpenShift Route CRUD/admission utilities via dynamic client. |
| pkg/k8s/route_test.go | Unit tests for k8s Route utilities (ensure/remove/admit/read). |
| pkg/k8s/lister.go | Uses Service annotation to surface external HTTPS URL when exposed. |
| pkg/k8s/describer.go | Reports external+internal routes when exposed (via Service annotation). |
| pkg/k8s/deployer.go | Adds exposure resolution (Route ensure/remove + Service annotation) for raw deployer; refactors resource-check messaging. |
| pkg/k8s/deployer_test.go | Adds tests for exposure gating and referenceCheckMessage. |
| pkg/functions/function.go | Adds DeploySpec.Expose and validates it in Function.Validate(). |
| pkg/functions/function_expose.go | Adds ValidateExpose + validateExpose (library + validation errors). |
| pkg/functions/function_expose_unit_test.go | Unit tests for expose validation and Function.Validate integration. |
| pkg/functions/errors.go | Adds ErrInvalidExpose. |
| pkg/functions/client.go | Updates deploy success message wording (removes “exposed”). |
| pkg/functions/client_test.go | Tests that Deploy prints success message containing namespace + URL. |
| pkg/describer/testing/integration_test_helper.go | Forces integration test deploys to be cluster-local (Expose: "none"). |
| pkg/deployer/testing/integration_test_helper.go | Forces multiple integration test deploys to be cluster-local (Expose: "none"). |
| go.mod | Bumps sigs.k8s.io/gateway-api indirect dependency to v1.5.1. |
| go.sum | Updates checksums for the gateway-api bump. |
| e2e/e2e_trigger_sync_test.go | Ensures raw deploys in e2e are cluster-local (--expose none). |
| e2e/e2e_metadata_test.go | Ensures raw deploys in e2e are cluster-local (--expose none). |
| docs/reference/func_deploy.md | Documents new --expose flag. |
| cmd/errors.go | Wraps invalid expose values with a typed CLI error (help text). |
| cmd/deploy.go | Adds --expose flag, validation, completion, and warning when ignored. |
| cmd/deploy_test.go | Adds CLI tests for expose persistence/clearing, invalid values, and ignore warning. |
| cmd/completion_util.go | Adds shell completion for --expose. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
86e9637 to
d2880a9
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 36 out of 36 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (4)
pkg/mock/deployer.go:44
- pkg/mock/NewDeployer always returns Status=Deployed on success, even when redeploying an already-deployed function (f.Deploy.Namespace set). This makes the mock less faithful and can hide update-path behavior (e.g., status messages that differ for Updated). Set Status=Updated when f.Deploy.Namespace is non-empty and no explicit new namespace was requested.
schema/func_yaml-schema.json:140 - schema/func_yaml-schema.json is listed as a generated file in AGENTS.md, so it shouldn’t be edited manually. Please regenerate it via the project’s generator (e.g. ./hack/update-codegen.sh / make all) and commit the regenerated output, to avoid hand-edited drift.
pkg/keda/lister.go:86 - This creates a new dynamic client inside Lister.get() for every listed function on OpenShift, which can be unnecessarily expensive (config loading/client construction in a tight loop). Consider creating the dynamic client once in List() (when IsOpenShift), reusing it across items, and passing it into get() (or storing it on the Lister).
if k8s.IsOpenShift() {
if dynClient, err := k8s.NewDynamicClient(); err == nil {
routeHost, routeFound, _ = k8s.GetAdmittedRouteHost(ctx, dynClient, interceptorNamespace(), interceptorRouteName(name, namespace))
}
}
pkg/k8s/deployer.go:368
- When Route removal is forbidden and enforce=false, this warns that the Route is being left in place, but then still clears RouteHostnameAnnotation. That makes list/describe report the function as not externally exposed even though the Route likely still exists (and you explicitly said you’re leaving it). In the forbidden/non-enforced path, skip clearing the annotation (or only clear it when the Route was actually removed).
if _, err := RemoveManagedRoute(ctx, dynClient, namespace, name, KubernetesDeployerName); err != nil {
if !enforce && errors.IsForbidden(err) {
fmt.Fprintf(os.Stderr, "⚠️ cannot remove Route %q (forbidden) - leaving it in place\n", name)
} else {
return fmt.Errorf("failed to remove Route: %w", err)
}
}
🤣 🤖 |
Changes
/kind
Fixes #
Release Note
Docs