ocp-route: new WithExposer() on deployer impl - #3991
Conversation
|
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 |
42fe8b6 to
2402347
Compare
There was a problem hiding this comment.
Pull request overview
This PR introduces an explicit external exposure model (--expose) for raw and keda deployments, backed by a new OpenShift Route “exposer” implementation, and threads the intent/observed exposure state through deploy, describe/list, remove, Tekton pipelines, tests, and CI/E2E coverage.
Changes:
- Add
Function.Expose(intent) andDeploySpec.Expose(observed/applied) plus validation, CLI flag + completion, and user-facing output updates. - Implement OpenShift Route exposure via new
pkg/ocprouteand wire it into raw/keda deploy/remove flows (including persistence of exposure records on the function Service). - Expand unit/integration/E2E coverage and add a dedicated GitHub Actions job for the exposure E2E subset.
Reviewed changes
Copilot reviewed 53 out of 53 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| schema/func_yaml-schema.json | Schema updates for new expose fields (generated). |
| pkg/remover/testing/integration_test_helper.go | Integration test fixture update for exposure. |
| pkg/pipelines/tekton/pipelines_provider.go | Record observed exposure from pipeline describer; adjust status messaging. |
| pkg/ocproute/route.go | New OpenShift Route exposer implementation. |
| pkg/ocproute/route_test.go | Unit tests for ocproute exposer behavior. |
| pkg/mock/remover.go | Mock remover signature updated to include Function. |
| pkg/mock/deployer.go | Mock deployer returns observed exposure when intent is active. |
| pkg/lister/testing/integration_test_helper.go | Integration test fixture update for exposure. |
| pkg/knative/remover.go | Remover signature updated to include Function. |
| pkg/knative/deployer.go | Neutralize “exposed at URL” wording in verbose output. |
| pkg/keda/remover.go | Add exposer wiring + remove Route on delete using recorded namespace. |
| pkg/keda/remover_unit_test.go | Unit tests for keda remover Route cleanup logic. |
| pkg/keda/lister.go | Prefer recorded exposed hostname rather than re-looking up exposure. |
| pkg/keda/exposure.go | New shared keda exposure helpers (namespace resolution, URL selection, validation). |
| pkg/keda/exposure_test.go | Unit tests for keda exposure helpers. |
| pkg/keda/describer.go | Report exposed URL first and record applied exposure in Instance. |
| pkg/keda/deployer.go | Wire exposer, validate names, reconcile exposure, record hostname/namespace on Service. |
| pkg/k8s/security_context_test.go | Update SetOpenShiftForTest signature usage. |
| pkg/k8s/remover.go | Remover signature updated to include Function. |
| pkg/k8s/openshift.go | OpenShift detection now reports (bool, error) via discovery of Route API. |
| pkg/k8s/openshift_unit_test.go | Goimports formatting cleanup. |
| pkg/k8s/lister.go | Prefer external https URL when Route hostname annotation exists. |
| pkg/k8s/labels/labels.go | Add FunctionNamespaceKey + FunctionKey constants for selectors. |
| pkg/k8s/describer.go | Report external URL first and include both external+internal routes when exposed. |
| pkg/k8s/deployer.go | Add exposure reconciliation, Service annotations, selector label filtering, trigger ownerref tweaks. |
| pkg/k8s/deployer_test.go | Unit tests for exposure reconciliation and namespace rule sharing. |
| pkg/functions/function.go | Add Function.Expose and DeploySpec.Expose plus validation hook. |
| pkg/functions/function_expose.go | New exposure constants + validation helpers. |
| pkg/functions/function_expose_unit_test.go | Unit tests for exposure validation helpers. |
| pkg/functions/errors.go | Add ErrInvalidExpose. |
| pkg/functions/client.go | Validate expose intent; persist observed exposure; remover API updated; improved messaging. |
| pkg/functions/client_test.go | Tests for invalid expose and deploy output; switch policy update. |
| pkg/describer/testing/integration_test_helper.go | Integration test fixture update for exposure. |
| pkg/deployers/deployers.go | Block all cross-deployer redeploy switches. |
| pkg/deployers/deployers_test.go | Updated test coverage for stricter switch policy. |
| pkg/deployer/testing/integration_test_helper.go | Integration test fixtures updated for exposure. |
| pkg/deployer/expose.go | New shared Exposer interface + Exposure/ExposureRef types. |
| pkg/deployer/common.go | Add DomainLabel const + SelectorLabels helper (exclude domain from selectors). |
| Makefile | Add build-tag vetting + expose E2E target; refine goimports check. |
| e2e/e2e_trigger_sync_test.go | Ensure raw deploys in this suite are cluster-local. |
| e2e/e2e_test.go | Only force FUNC_NAMESPACE when explicitly set; add newCmdOutput helper. |
| e2e/e2e_metadata_test.go | Ensure raw deploy is cluster-local in metadata test. |
| e2e/e2e_expose_test.go | New E2E suite for expose semantics and platform gating. |
| docs/reference/func_deploy.md | Document --expose and domain behavior for routes. |
| cmd/func-util/main.go | Wire ocproute exposer into func-util deployers and add OpenShift gate. |
| cmd/errors.go | Add typed CLI error wrapper for invalid expose values. |
| cmd/describe.go | Mark routes as “(exposed)” vs “(cluster-local)” for raw/keda. |
| cmd/describe_test.go | Unit tests for route marker behavior. |
| cmd/deploy.go | Add --expose, validation, platform gate, and warnings; refactor deployer option selection. |
| cmd/deploy_test.go | Add CLI tests for expose persistence, invalid values, warnings, and remote observation. |
| cmd/completion_util.go | Shell completion for --expose values. |
| cmd/client.go | Wire ocproute exposer into raw/keda deployers and keda remover. |
| .github/workflows/functions.yaml | Add CI job running expose E2E tests on KinD. |
Suppressed comments (6)
pkg/deployer/testing/integration_test_helper.go:328
- DeploySpec.Expose is observed state and won’t affect deployment intent; use Function.Expose to opt out of exposure in these integration tests.
Deploy: fn.DeploySpec{Expose: "none"},
pkg/deployer/testing/integration_test_helper.go:172
- DeploySpec.Expose is observed state and won’t affect deployment intent; use Function.Expose to opt out of exposure in these integration tests.
Deploy: fn.DeploySpec{Expose: "none"},
pkg/deployer/testing/integration_test_helper.go:412
- DeploySpec.Expose is observed state and won’t affect deployment intent; use Function.Expose to opt out of exposure in these integration tests.
Deploy: fn.DeploySpec{Expose: "none"},
pkg/deployer/testing/integration_test_helper.go:528
- DeploySpec.Expose is observed state and won’t affect deployment intent; use Function.Expose to opt out of exposure in these integration tests.
Deploy: fn.DeploySpec{Expose: "none"},
pkg/deployer/testing/integration_test_helper.go:940
- DeploySpec.Expose is observed state and won’t affect deployment intent; use Function.Expose to opt out of exposure in these integration tests.
Deploy: fn.DeploySpec{Expose: "none"},
pkg/deployer/testing/integration_test_helper.go:1248
- DeploySpec.Expose is observed state and won’t affect deployment intent; use Function.Expose to opt out of exposure in these integration tests.
Deploy: fn.DeploySpec{Expose: "none"},
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
2402347 to
506f60c
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 51 out of 51 changed files in this pull request and generated 2 comments.
Suppressed comments (2)
pkg/keda/deployer.go:187
- When an exposed function is redeployed after the interceptor moves namespaces (for example, upstream
kedato CMA'sopenshift-keda), this creates a Route in the newly detected namespace while the Service still records the old namespace. Because active exposure skips the teardown branch and later overwrites that record, the old ownerless Route is permanently orphaned. Reconcile a differing recorded namespace by removing the old Route after the new Route/HSO is ready and before replacing the annotation.
pkg/functions/client.go:932 - This warning also fires for the Knative deployer when
expose: routeis present: Knative intentionally leavesresult.Exposeempty because it ignores this field, while still exposing the function through its own networking. That produces a second, false warning afterwarnExposeIgnore. Match the remote-deploy path and exclude Knative results here.
if ActiveExpose(f.Expose) && result.Expose == "" {
506f60c to
f0cbb0b
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 52 out of 52 changed files in this pull request and generated 4 comments.
Suppressed comments (1)
pkg/ocproute/route.go:412
- A rejection from one router ends polling immediately, even when another ingress entry is still pending and may subsequently become
Admitted=True. Since router shards update status independently, this makes list timing determine whether a valid multi-router Route succeeds. Keep the rejection aslastErr, but continue polling until an admission arrives (or until rejection can be established for every expected shard).
f0cbb0b to
9beec4b
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 53 out of 53 changed files in this pull request and generated 1 comment.
Suppressed comments (3)
pkg/keda/deployer.go:200
- After
Exposesucceeds, an HTTPScaledObject create/update/readiness failure returns here without removing a newly admitted Route. That keda Route is ownerless and has not yet been recorded on the Service, so a later--expose=noneskips it; if the HSO becomes ready after the timeout, the unrecorded address can serve traffic. Roll back Routes created in this attempt (while leaving a previously recorded Route intact) and report any rollback failure.
cmd/errors.go:238 - This names the wrong configuration field: users request exposure through top-level
expose/--expose, whiledeploy.exposeis observed state written after deployment. Referring users todeploy.exposeencourages editing a status field that does not control deployment.
deploy.expose takes effect with the raw and keda deployers only (--deployer=raw or --deployer=keda).
pkg/keda/deployer.go:221
- This overwrites the only Route-location record without reconciling its previous value. If the interceptor moves between the supported
kedaandopenshift-kedanamespaces, an active redeploy creates a Route in the new namespace but leaves the old ownerless Route behind; subsequent delete follows only the new record, so the old Route survives indefinitely. Reconcile the previously recorded namespace when it differs before replacing the record, with rollback if cleanup fails.
9beec4b to
e590351
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 53 out of 53 changed files in this pull request and generated 1 comment.
Suppressed comments (4)
pkg/keda/deployer.go:199
- If Route creation succeeds but
ensureHTTPScaledObjectfails (including a readiness timeout), this returns without removing the ownerless KEDA Route or recording its namespace on the Service. The Route is then invisible to the normal--expose=nonepath and may begin serving later if the HTTPScaledObject becomes ready. Roll back newly created exposure on this failure while preserving a previously recorded Route.
pkg/pipelines/tekton/pipelines_provider.go:284 - An empty
obj.Exposedoes not prove a stale func-util removed an existing raw Route. A pre-feature raw deployer updates the Service with a regenerated annotation map, stripping the new exposure record, but it never deletes the Service-owned Route; this then records cluster-local state and warns accordingly while the Route still serves. When the prior state was exposed and the observed annotation disappears, verify the managed Route is absent or remove it before clearingDeploy.Expose.
pkg/k8s/deployer.go:315 - This error assumes the mismatched selector key is always the domain, but legacy Deployments selected on every common and user label. Changing a runtime or deploy label therefore reaches this branch and receives incorrect domain-specific guidance. Make the recreation instruction generic to the reported pinned selector key.
"function %q cannot be updated in place: its Deployment was created by an older func whose selector pins %s=%q, and this deploy no longer carries that label. Changing or removing a pinned domain needs recreation: run 'func delete' and deploy again",
fnName, k, v)
cmd/errors.go:238
- The user-configurable intent is the top-level
exposefield;deploy.exposeis observed state written by the deployer. Namingdeploy.exposehere directs users to edit the wrong field.
deploy.expose takes effect with the raw and keda deployers only (--deployer=raw or --deployer=keda).
e590351 to
35c4b1a
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 53 out of 53 changed files in this pull request and generated 1 comment.
Suppressed comments (4)
pkg/keda/deployer.go:199
- At this point an active deploy has already created or replaced the ownerless keda Route. If HTTPScaledObject creation/update or readiness fails, this return leaves that Route live and unrecorded, so opt-out cannot find it and deletion only discovers it via a later best-effort sweep. Roll back a newly created/replaced Route on this failure while preserving a pre-existing serving Route.
pkg/keda/deployer.go:324 - This unconditionally removes the Route after any record-write failure, including when
Exposemerely updated a pre-existing serving Route during redeploy. A transient Service failure therefore takes down an established endpoint. Propagate whether the Route was newly created/replaced and roll back only in that case.
pkg/keda/deployer.go:187 - Active redeploys always create/find the Route in the currently probed interceptor namespace, but ignore the namespace already recorded on
appService. If the installation moves betweenkedaandopenshift-keda, this creates a second ownerless Route and overwrites the record, so the old Route is never removed (and a custom host cannot migrate because the old Route still owns it). Reconcile a differing recorded namespace explicitly before replacing the record.
pkg/k8s/deployer.go:426 - This rollback also deletes a pre-existing Route on an ordinary redeploy.
Exposedoes not report whether it created the object, so a transient Service read/update failure after updating an existing serving Route takes that public endpoint down, even though its previous exposure record may still be valid. Only roll back when this call actually created or replaced the Route.
if rbErr := d.exposer.Unexpose(ctx, dynClient, e.Ref()); rbErr != nil {
Functions deployed with the raw and keda deployers can now get a public URL through an OpenShift Route.
Removed the "slightly convenient" deployer switch raw -> keda allowance -> all deployer switching now prints an error that resources would be stranded and to undeploy first
raw deployer adds Route to function's namespace, adds ownerReference, all related resources live as long as function service exists and/or is redeployed otherwise.
keda deployer deploys the Route in keda-derived namespace (
kedaoropenshift-keda) because it needs to target the intereceptor in order for keda's scaling to work. (Route cannot target services across namespaces) - ownerRef also cannot target across namespaces -> this requires explicit deletion.we add selectors (what does not change for a deployed function), labels and annotations to identify the function and if it was deployed/how. - this is convenient so that commands like
listanddescribecan only fetch the function service as usual and learn all about it via its metadata, no need to fetch other resources around the cluster.AI info
Usage
--expose=""clears it.func describeleads with the public URL of an exposed function.--expose=routeis refused before anything is built, with a distinct message when the cluster could not be reached. The knative deployer ignores the flag (it has its own exposure) with a warning.func deleteremoves the exposure with the function, including keda's Route, which nothing garbage collects.How it works
Mechanism, for reviewers
pkg/deployer.Exposeris a small pluggable interface;pkg/ocprouteimplements it through the dynamic client (nogithub.com/openshift/apidependency).BlockOwnerDeletion, which OpenShift'sOwnerReferencesPermissionEnforcementwould reject for ordinary users.--expose=noneand onfunc delete, with a best-effort candidate-namespace sweep for Routes a crash left unrecorded. A namespace the account cannot read is warned about and skipped, so deleting a never-exposed function needs no interceptor-namespace permissions.openshift-kedafor CMA,kedafor the upstream chart). Exposure refuses when the probe cannot confirm one; cluster-local deploys proceed on the platform's guess.expose) and applied state (deploy.expose) separately. Remote deploys record what the pipeline's describer observed, never the intent: a published func-util that predates this feature deploys cluster-local, and the CLI warns instead of recording a Route that does not exist. A Route whose record cannot be written is rolled back within the same deploy.func.domainlabel is excluded from the immutable Deployment and Service selectors; updates preserve a legacy Deployment's live selector, and changing a domain pinned by one refuses with recreation instructions. A domain change replaces the Route (in-place host edits needroutes/custom-hostupdate permission project admins lack; the old certificate names the old host). Updates reconcile func's owned fields onto the live Route, so third-party state survives.Testing
BlockOwnerDeletionadmission).make test-e2e-expose).🤖 Generated with Claude Code