design-proposal: serverless (FaaS + event-driven) as managed apps#31
design-proposal: serverless (FaaS + event-driven) as managed apps#31IvanHunters wants to merge 1 commit into
Conversation
Signed-off-by: IvanHunters <xorokhotnikov@gmail.com>
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a design proposal for adding serverless capabilities (FaaS and event-driven autoscaling) to Cozystack as managed applications using Knative and KEDA. The review feedback suggests several key improvements to the design: making the Kubernetes Service optional for KEDA Worker workloads while addressing trigger authentication, explicitly excluding Gateway API CRDs from the Knative controller package to prevent resource ownership conflicts with Cilium, and disabling service account token automounting by default on tenant workloads to minimize the security attack surface.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| - **`Worker`** (KEDA; name in Open questions) — event-driven. The wrapper is | ||
| thicker because KEDA only *scales* an existing workload, so the chart assembles | ||
| `Deployment` + `Service` + `ScaledObject`. Values: image, trigger | ||
| (queue/Kafka/cron/Prometheus), `minReplicaCount` (0 for scale-to-zero), | ||
| `maxReplicaCount`, env, resources. |
There was a problem hiding this comment.
A standard event-driven worker pulling from a queue or event source (such as Kafka, RabbitMQ, or SQS) does not expose any ports and does not require a Kubernetes Service. Creating a Service by default for all Worker instances is unnecessary and can be avoided or made optional to prevent unused resource allocation. Additionally, secured event sources require credentials to allow KEDA to poll for metrics. The design should specify how KEDA's TriggerAuthentication or ClusterTriggerAuthentication resources will be integrated or exposed to the tenant.
| - **`Worker`** (KEDA; name in Open questions) — event-driven. The wrapper is | |
| thicker because KEDA only *scales* an existing workload, so the chart assembles | |
| `Deployment` + `Service` + `ScaledObject`. Values: image, trigger | |
| (queue/Kafka/cron/Prometheus), `minReplicaCount` (0 for scale-to-zero), | |
| `maxReplicaCount`, env, resources. | |
| - **`Worker`** (KEDA; name in Open questions) — event-driven. The wrapper is | |
| thicker because KEDA only *scales* an existing workload, so the chart assembles | |
| `Deployment` + `ScaledObject` (with an optional `Service` if required for metrics/scraping). | |
| Values: image, trigger (queue/Kafka/cron/Prometheus), `minReplicaCount` (0 for scale-to-zero), | |
| `maxReplicaCount`, env, resources, and optional trigger authentication. |
| - `cozystack.keda-operator` / `cozystack.knative-controller` — the engine; | ||
| `dependsOn: [networking]` (+ `cert-manager` where a webhook needs it); installs | ||
| into its own `cozy-*` namespace. |
There was a problem hiding this comment.
Bundling or overwriting cluster-wide Gateway API CRDs in the Knative controller package when they are already managed by the networking (Cilium) package causes resource ownership conflicts and upgrade failures in Flux. The package structure should explicitly exclude these CRDs.
| - `cozystack.keda-operator` / `cozystack.knative-controller` — the engine; | |
| `dependsOn: [networking]` (+ `cert-manager` where a webhook needs it); installs | |
| into its own `cozy-*` namespace. | |
| - `cozystack.keda-operator` / `cozystack.knative-controller` — the engine; | |
| `dependsOn: [networking]` (+ `cert-manager` where a webhook needs it); installs | |
| into its own `cozy-*` namespace. The Knative controller package should explicitly | |
| exclude or disable the installation of Gateway API CRDs to prevent ownership conflicts | |
| with the `networking` (Cilium) package. |
| - **Default placement (tenant namespace).** Like every other managed app, the | ||
| workload runs in the tenant namespace on management nodes. This inherits the | ||
| tenant's existing Cilium network policy, RBAC and quotas, but the pods share the | ||
| management host kernel — the same exposure any tenant pod has today. |
There was a problem hiding this comment.
Arbitrary tenant-supplied code running in the tenant namespace has access to the default ServiceAccount token if it is automounted. Disabling service account token automounting (automountServiceAccountToken: false) by default on the pod templates for both Function and Worker workloads reduces the attack surface inside the tenant namespace.
| - **Default placement (tenant namespace).** Like every other managed app, the | |
| workload runs in the tenant namespace on management nodes. This inherits the | |
| tenant's existing Cilium network policy, RBAC and quotas, but the pods share the | |
| management host kernel — the same exposure any tenant pod has today. | |
| - **Default placement (tenant namespace).** Like every other managed app, the | |
| workload runs in the tenant namespace on management nodes. This inherits the | |
| tenant's existing Cilium network policy, RBAC and quotas, but the pods share the | |
| management host kernel — the same exposure any tenant pod has today. To minimize | |
| the attack surface, the pod templates for both `Function` and `Worker` workloads | |
| should disable service account token automounting (`automountServiceAccountToken: false`) | |
| by default. |
myasnikovdaniil
left a comment
myasnikovdaniil
left a comment
There was a problem hiding this comment.
NOT LGTM (as a design draft) — the direction is sound and the load-bearing facts check out against main (Cilium is genuinely 1.19.5; KEDA appears only in the ingress-nginx subchart; the Postgres two-PackageSource pattern is described correctly). One architectural contradiction should be resolved before this guides implementation; the rest are design-completeness gaps left inline.
Blocker — ComputePlane placement vs controller topology. §1 runs the engines cluster-wide in the management cluster; Security recommends placing untrusted workloads on a ComputePlane (#17), which is a separate Cozystack-managed cluster reconciled by one-way remote Flux apply. A management-cluster controller cannot reconcile CRs that live in a different cluster, so the recommended hardening — the proposal's whole answer for arbitrary tenant code — isn't implementable with the stated topology. Details inline.
Non-blocking (inline): KEDA trigger credentials unmodeled; Worker renders a Service unconditionally; knative-controller must not vendor Gateway API CRDs; disable/rollback is destructive with live functions; default automountServiceAccountToken: false; migration numbers already consumed on main; confirm Cilium fix-PRs in the spike.
One concern I investigated and dismissed: the Knative activator/autoscaler → tenant-pod path is not blocked by the tenant CiliumNetworkPolicy — allow-external-communication permits ingress fromEntities: [world, cluster]. (Tenant egress is restricted, so the spike should confirm any pod→controller-namespace flow.)
| workload runs in the tenant namespace on management nodes. This inherits the | ||
| tenant's existing Cilium network policy, RBAC and quotas, but the pods share the | ||
| management host kernel — the same exposure any tenant pod has today. | ||
| - **Recommended hardening (ComputePlane placement, #17).** Because the workload is |
There was a problem hiding this comment.
This is the core tension. §1 puts the KEDA operator / Knative controller cluster-wide in the management cluster, but a ComputePlane (#17) is a separate Cozystack-managed cluster, reconciled by one-way remote Flux apply. Under ComputePlane placement the ScaledObject/Knative Service CR lands in the ComputePlane cluster while the controller that must reconcile it (admission webhook, sidecar injection, scaling) runs in the management cluster — and a controller only watches its own cluster's API server. For ComputePlane placement to work, the engines would need to be installed on each ComputePlane as an addon of the kubernetes substrate (the way that app already installs cert-manager/ingress-nginx/CNI via remote Flux), not as a management-cluster singleton. The Postgres analogy hides this because CNPG's operator and its pods share one cluster and Postgres is never placed on a ComputePlane. Since ComputePlane placement is the proposal's answer for arbitrary tenant code, please resolve explicitly: where do the engines run under ComputePlane placement, and how does that square with "trusted controller = management singleton"? The N-ComputePlanes × engine-install lifecycle/versioning is worth covering too.
| thicker because KEDA only *scales* an existing workload, so the chart assembles | ||
| `Deployment` + `Service` + `ScaledObject`. Values: image, trigger | ||
| (queue/Kafka/cron/Prometheus), `minReplicaCount` (0 for scale-to-zero), | ||
| `maxReplicaCount`, env, resources. |
There was a problem hiding this comment.
The Worker value set has no credential path. Most real event sources — Kafka+SASL, RabbitMQ, cloud queues, authenticated Prometheus — require secrets for KEDA to poll, which KEDA models via TriggerAuthentication/ClusterTriggerAuthentication + secret refs. Without a first-class auth field the Worker only serves unauthenticated triggers. Worth modeling explicitly in the CR.
|
|
||
| The one real difference between engines: Knative's `Service` is already | ||
| high-level, so `Function` is a thin wrapper. KEDA does not deploy a workload, so | ||
| `Worker` templates `Deployment` + `Service` + `ScaledObject` together. Both are |
There was a problem hiding this comment.
A queue/cron/Kafka worker exposes no ports and needs no Service; rendering one unconditionally allocates an unused object per Worker. Suggest making the Service opt-in — only when a trigger needs a scrape/metrics endpoint.
|
|
||
| Two PackageSources per engine: | ||
|
|
||
| - `cozystack.keda-operator` / `cozystack.knative-controller` — the engine; |
There was a problem hiding this comment.
Worth an explicit design constraint: the knative-controller package must not vendor/install the Gateway API CRDs. Those are owned by the Cilium networking package; a second package shipping them collides on ownership and wedges Flux upgrades. Since the design reuses the existing Cilium Gateway API, exclude the CRDs and dependsOn: [networking].
| - **Enable/disable:** flipping the bundle toggle adds/removes the engine and the | ||
| tenant CRs' `HelmRelease`s. Standard HR lifecycle; no controller cooperation | ||
| beyond Flux. | ||
| - **Rollback:** disabling removes both; tenant CRs are garbage-collected. |
There was a problem hiding this comment.
"tenant CRs are garbage-collected" understates this. If tenants have live Function/Worker objects, flipping the bundle off removes the ApplicationDefinition/CRDs and their rendered workloads — silent loss of running tenant workloads, not a no-op. Recommend documenting a guard (refuse-if-in-use / drain) rather than presenting disable as clean.
| tenant-supplied code** — unlike Postgres, which runs a fixed image the tenant | ||
| cannot execute code inside. Two layers: | ||
|
|
||
| - **Default placement (tenant namespace).** Like every other managed app, the |
There was a problem hiding this comment.
For arbitrary-code workloads under the default (tenant-namespace) placement, defaulting automountServiceAccountToken: false on the function/worker pod templates is cheap attack-surface reduction. Worth stating as a platform-owned default.
| - **Backward compatible:** with the apps disabled, no serverless objects render — | ||
| identical to `main`; helm-unittest covers the 0-document case. | ||
| - **Registration on existing clusters:** a numbered migration registers the new | ||
| PackageSources — migration `50`, bump `targetVersion` to 51. |
There was a problem hiding this comment.
These numbers are already consumed on main: the last migration file is 50 and migrations.n is 51. So "migration 50" collides and "bump targetVersion to 51" is already current. For a design doc, phrase it as "the next free migration slot" so it doesn't read as a concrete (now-wrong) instruction.
| `192.192.192.192:9999`, so the ingress probe hung `Uninitialized` | ||
| (knative-extensions/net-gateway-api#817, closed *not-planned* on the Knative | ||
| side). Fixed **in Cilium** via community PRs cilium/cilium#46237 (+ #46242 / | ||
| #46296 / #46127), backported through #46400, **shipped in v1.19.5** (release |
There was a problem hiding this comment.
The 1.19.5 version claim checks out (vendored chart appVersion: 1.19.5, recent bump commit). I didn't independently verify the specific fix-PR numbers cited — but since the design gates the Knative release on a confirmation spike with an Envoy Gateway/Contour fallback, that's fine; just fold "confirm the exact fix commits are present in 1.19.5" into the spike's acceptance criteria.
2 participants
Summary
Adds a design proposal for a serverless capability in Cozystack, delivered as tenant-facing managed apps.
It covers the two distinct things commonly called "serverless":
Approach
Each engine follows the standard managed-app split (as Postgres/CNPG and LINSTOR/piraeus-operator do):
cozy-*namespace;Knative Service,Deployment+ScaledObject) render into the tenant namespace;Function/Worker) wrapped by anApplicationDefinition.Untrusted-code isolation (a function runs arbitrary tenant code) is left to placement on a ComputePlane (#17) as an orthogonal, referenced concern rather than something re-specified here.
Notes
Draft, opened for discussion.