design-proposal(compute-plane): deliver as an operator-owned extra module wrapping apps/kubernetes (supersedes #27)#33
Conversation
📝 WalkthroughWalkthroughChangesThe ComputePlane proposal now defines an operator-owned Tenant module wrapping ComputePlane delivery redesign
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 updates the design proposal for Cozystack's ComputePlane. Instead of introducing a new top-level kind or user-facing preset fields on kind: Kubernetes, the proposal now delivers ComputePlane as an operator-controlled Tenant module (packages/extra/computeplane) that wraps the ordinary apps/kubernetes chart with operator-fixed values. The reviewer identified an architectural contradiction in the packaging design: the proposal describes ComputePlane as a new Tenant module, but the illustrative HelmRelease points directly to the re-sourced apps/kubernetes chart, which would bypass the packages/extra/computeplane chart entirely. Clarification on whether the module chart acts as a wrapper or if it is redundant is requested.
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.
| Reuse the `kubernetes` app as the substrate, unchanged. A ComputePlane is delivered as a new Tenant module, `packages/extra/computeplane`, of the same shape as `extra/etcd` / `extra/seaweedfs`. When a tenant has the module enabled, the tenant chart renders a `HelmRelease` (in the tenant namespace, on the management cluster) whose `chartRef` points at the **source-only** `apps/kubernetes` `ExternalArtifact`, carrying operator-fixed values: | ||
|
|
||
| ```yaml | ||
| # packages/extra/computeplane — rendered HelmRelease (illustrative) | ||
| apiVersion: helm.toolkit.fluxcd.io/v2 | ||
| kind: HelmRelease | ||
| metadata: | ||
| name: computeplane | ||
| namespace: tenant-<name> # management cluster, tenant namespace | ||
| spec: | ||
| chartRef: | ||
| kind: ExternalArtifact | ||
| name: cozystack-computeplane-application-kubevirt-kubernetes # apps/kubernetes, re-sourced (Design §2) | ||
| namespace: cozy-system | ||
| values: # operator-owned; the tenant cannot edit these | ||
| # hardened posture: restricted PSA + admission, deny egress → management kube-apiserver, | ||
| # scoped per-service egress only, addon set, GPU node groups, autoscaling bounds … | ||
| valuesFrom: | ||
| - kind: Secret # only the curated knobs the module chooses to surface | ||
| name: cozystack-values | ||
| ``` |
There was a problem hiding this comment.
There is an architectural contradiction in how the packages/extra/computeplane module is defined versus how it is deployed:
- The proposal states that "ComputePlane is delivered as a new Tenant module,
packages/extra/computeplane" (line 78). - However, the illustrative
HelmRelease(lines 81-98) has itschartRefpointing directly atcozystack-computeplane-application-kubevirt-kubernetes(which is the re-sourcedapps/kuberneteschart).
If the tenant chart's HelmRelease points directly to the re-sourced apps/kubernetes chart, the packages/extra/computeplane chart itself is bypassed and remains completely unused.
To resolve this, please clarify the packaging structure:
- Option A: The tenant chart renders a
HelmReleasepointing topackages/extra/computeplane, andpackages/extra/computeplaneacts as the wrapper that contains/renders theapps/kubernetesHelmReleaseor templates. - Option B: If the tenant chart directly releases
apps/kuberneteswith overridden values, then thepackages/extra/computeplanechart is redundant and only thecomputeplane-applicationPackageSource andcomputeplane-rdare needed.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@design-proposals/compute-plane/README.md`:
- Line 151: Expand the “Connectivity to tenant services” section to define the
complete ComputePlane-to-service egress contract: identify the authorization
owner, specify how scoped CiliumNetworkPolicy resources are generated, bind
policies to workloads and tenant services, define revocation behavior, and
prevent bypass through direct service endpoints. Keep the contract explicit
about denying kube-apiserver access and allowing only authorized tenant
services.
- Line 211: Clarify the tenant/module deletion flow by naming the existing
component that owns and removes the module finalizer, and document the
fail-closed disable-then-deprovision sequence. Explicitly state that remote
HelmReleases are deleted before the ComputePlane or target cluster disappears,
and describe the behavior when cleanup cannot be confirmed so deletion cannot
hang or proceed prematurely.
- Line 136: Update the ComputePlane design and related deployment flow so
placement: ComputePlane app releases use Flux remote-cluster ServiceAccount
impersonation via spec.serviceAccountName and a least-privilege identity. Remove
references to computeplane-admin-kubeconfig and super-admin.svc for app routing,
while retaining the Kamaji admin kubeconfig only where administrative access is
explicitly required.
- Around line 207-208: Update the ComputePlane placement dependency
documentation and implementation guidance so the remote HelmRelease waits for
the module-level ComputePlane Ready condition, not merely the
computeplane-admin-kubeconfig Secret. Ensure Flux starts the remote release only
after PSA, admission, and egress hardening reconciliation completes, while
preserving the existing fail-closed behavior for missing or rotated credentials.
- Around line 92-98: Update the cozystack-values valuesFrom configuration to
enforce an explicit allowlist or schema for accepted Secret keys, ensuring
parent- or tenant-editable inputs cannot override hardened settings through
targetPath or Flux merge ordering. Keep only the curated knobs intended by the
module exposed, and preserve the operator-owned inline values as authoritative.
- Line 144: Update the ComputePlane deployment flow for ApplicationDefinition
placement so the target tenant namespace is pre-created with required PSA
labels, quotas, and ownership metadata before the HelmRelease can install
workloads. Do not rely solely on spec.install.createNamespace; either provision
the namespace explicitly or enforce equivalent admission gating before workload
creation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9a3c7d2e-1201-4964-b71f-8b232983157f
📒 Files selected for processing (1)
design-proposals/compute-plane/README.md
| - `computeplane-rd` → `path: system/computeplane-rd` with `install:` — the resource-definition chart carrying the module's `cozyrd` (its `ApplicationDefinition` / dashboard presentation and the tenant-values wiring), the same role `seaweedfs-rd` / `kubernetes-rd` play. | ||
|
|
||
| An empty string means disabled; a non-empty value names a ComputePlane **profile/class**. The profile — node groups, GPU types, Kubernetes version, autoscaling bounds — is defined **once** at the tenant/platform level and referenced by name. There is deliberately **no per-module `valuesOverride` blob**: that would re-create the two-sources-of-truth problem (the module spec living both in the profile and inline on the tenant). Keeping the module a bare profile reference makes the profile definition the single source of truth. | ||
| The tenant chart gains a `computePlane` toggle (a bool, or a profile name if the operator ships more than one module variant) alongside the existing module toggles; when set by the parent tenant, `packages/apps/tenant/templates/computeplane.yaml` renders the `HelmRelease` above. Kamaji writes the cluster's admin kubeconfig to `computeplane-admin-kubeconfig` (key `super-admin.svc`), which `placement: ComputePlane` apps consume (Design §4). |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant section of the proposal
FILE="design-proposals/compute-plane/README.md"
wc -l "$FILE"
sed -n '120,170p' "$FILE"
# Find references to the kubeconfig and placement
rg -n "computeplane-admin-kubeconfig|super-admin\.svc|placement: ComputePlane|HelmRelease|serviceAccountRef|remote cluster" "$FILE"Repository: cozystack/community
Length of output: 16209
🌐 Web query:
Flux HelmRelease remote cluster serviceAccountName kubeConfig secretRef docs
💡 Result:
To deploy a HelmRelease to a remote cluster using Flux, you use the spec.kubeConfig field to reference a Kubernetes Secret containing the remote cluster's kubeconfig [1][2][3]. This configuration directs the Flux helm-controller to perform Helm operations on the target cluster instead of the local cluster where Flux is installed [2][4]. Key components for this setup include: 1. KubeConfig Secret: You must create a Kubernetes Secret in the same namespace as the HelmRelease [2][5][6]. This secret stores the kubeconfig file for the remote cluster, typically under the data key value (default) or as specified by the secretRef [2][4][5]. 2. HelmRelease Configuration: Include the kubeConfig field in your HelmRelease manifest to link to this secret [2][3]: spec: kubeConfig: secretRef: name: 3. Impersonation (Optional): You can optionally use spec.serviceAccountName to specify a ServiceAccount in the target cluster that the controller should impersonate [2][4]. When both spec.kubeConfig and spec.serviceAccountName are provided, the controller will act on behalf of that ServiceAccount within the remote cluster [2][4]. For this to work, the ServiceAccount must exist in the target cluster within a namespace that matches the namespace of the HelmRelease on the management cluster [2]. Important considerations: - The management cluster must have network access to the remote cluster's API server [5]. - If the remote API server uses self-signed certificates, you may need to configure the controller with the --insecure-kubeconfig-tls flag [5]. - The secretRef must reside in the same namespace as the HelmRelease object [2][5]. - If you do not specify a ServiceAccount, the controller will default to using the credentials provided in the kubeconfig [4]. Note that there is a distinction between the ServiceAccount used for impersonation on the remote cluster (spec.serviceAccountName) and the controller's own ServiceAccount [2].
Citations:
- 1: https://fluxcd.io/flux/components/helm/api/v2/
- 2: https://github.com/fluxcd/helm-controller/blob/main/docs/spec/v2/helmreleases.md
- 3: https://oneuptime.com/blog/post/2026-03-13-how-to-set-up-hub-and-spoke-mode-multi-cluster-with-flux-cd/view
- 4: https://github.com/fluxcd/helm-controller/blob/main/docs/spec/v2beta1/helmreleases.md
- 5: https://github.com/fluxcd/flux2-hub-spoke-example/blob/main/README.md
- 6: https://v2-0.docs.fluxcd.io/flux/components/helm/helmreleases/
Don't route app releases with the ComputePlane super-admin kubeconfig
placement: ComputePlane should use Flux remote-cluster ServiceAccount impersonation (spec.serviceAccountName) with a least-privilege identity, not computeplane-admin-kubeconfig/super-admin.svc. An admin kubeconfig gives a compromised chart the ability to create cluster-scoped resources and access other tenant secrets.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@design-proposals/compute-plane/README.md` at line 136, Update the
ComputePlane design and related deployment flow so placement: ComputePlane app
releases use Flux remote-cluster ServiceAccount impersonation via
spec.serviceAccountName and a least-privilege identity. Remove references to
computeplane-admin-kubeconfig and super-admin.svc for app routing, while
retaining the Kamaji admin kubeconfig only where administrative access is
explicitly required.
Source: MCP tools
| ### 4. `placement` routes a catalog app onto the ComputePlane | ||
|
|
||
| This is where the design leans entirely on an existing, proven mechanism. An application that must be isolated declares `placement: ComputePlane` on its `ApplicationDefinition`. When a tenant creates the app: | ||
| `ApplicationDefinition` gains `placement` = `ManagementPlane` (default) | `ComputePlane`. `ManagementPlane` deploys into the tenant namespace on the management cluster, as today. `ComputePlane` routes the generated `HelmRelease` onto the tenant's ComputePlane by injecting `spec.kubeConfig.secretRef` → `computeplane-admin-kubeconfig` (the module's fixed secret) and `spec.install.createNamespace: true`. Because the module is a single per-tenant enabler, `ComputePlane` resolves unambiguously to *that* tenant's cluster — no cluster name to thread through the app. |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
sed -n '120,170p' design-proposals/compute-plane/README.mdRepository: cozystack/community
Length of output: 7627
🏁 Script executed:
rg -n "createNamespace|targetNamespace|computeplane-admin-kubeconfig|Placement|ManagementPlane|ComputePlane" design-proposals/compute-plane/README.mdRepository: cozystack/community
Length of output: 19897
🌐 Web query:
Flux HelmRelease install.createNamespace targetNamespace namespace labels PSA quotas documentation
💡 Result:
Flux HelmRelease fields/behavior for: 1) install.createNamespace - In Flux HelmRelease v2, namespace creation is controlled by.spec.install.createNamespace (boolean). It “tells the Helm install action to create the HelmReleaseSpec.TargetNamespace if it does not exist yet”. On uninstall, “the namespace will not be garbage collected”. It defaults to false. [1][2] - Nuance: this only affects the Helm install action (not upgrades). [1][2][3] 2) targetNamespace -.spec.targetNamespace is optional and specifies “the namespace to which the Helm release is made”; it defaults to the namespace where the HelmRelease resource lives. [4][2] - Warning: changing.spec.targetNamespace after install will not move the already-installed release; instead it uninstalls and reinstalls in the new target namespace. [2] 3) namespace labels - Flux/helm-controller can apply labels/annotations to the Kubernetes objects it manages via: a) HelmRelease metadata (Kubernetes-native labels), b) HelmReleaseSpec.commonMetadata (“common labels and annotations that are applied to all resources”). [1] - Those labels are applied to the objects created by Flux/Helm (deployments, services, etc.), not automatically to the Namespace object itself via createNamespace. 4) PSA labels and quotas - Flux’s HelmRelease documentation does not state any built-in option that, when createNamespace is enabled, will automatically set Kubernetes Namespace PSA labels (pod-security.kubernetes.io/*) or create ResourceQuota/LimitRange objects for quotas. - PSA is enforced by Namespace-level labels, and quotas are represented by Kubernetes ResourceQuota/LimitRange API objects, which must be created as Kubernetes manifests (either by your GitOps namespace-manifest layer, and/or by templates in your Helm chart). [5][1][2] What this means for your specific query - install.createNamespace: use spec.install.createNamespace to ensure the target Namespace exists. [1][2] - targetNamespace: use spec.targetNamespace to choose where the Helm release is installed. [4][2] - labels / PSA quotas: you must manage Namespace labels for PSA and Namespace ResourceQuota/LimitRange objects separately from Flux’s createNamespace behavior; Flux’s HelmRelease createNamespace does not provide automatic PSA/quotas setup per the official docs. [5][1][2] If you tell me the PSA profile you want (e.g., baseline/restricted/audit/warn/enforce versions) and the quota numbers, I can generate a GitOps-ready set of Namespace + ResourceQuota/LimitRange manifests to pair with your HelmRelease.
Citations:
- 1: https://v2-0.docs.fluxcd.io/flux/components/helm/api/
- 2: https://github.com/fluxcd/helm-controller/blob/main/docs/spec/v2/helmreleases.md
- 3: Not creating new namespace with impersonation ServiceAccount even though it has proper role. fluxcd/helm-controller#338
- 4: https://fluxcd.io/flux/components/helm/helmreleases/
- 5: https://kubernetes.io/docs/concepts/security/pod-security-admission/
Pre-create the ComputePlane namespace. install.createNamespace: true only creates targetNamespace; it won’t apply PSA labels, quotas, or ownership metadata. Pre-create that namespace with the required controls, or gate namespace creation through admission before workloads start.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@design-proposals/compute-plane/README.md` at line 144, Update the
ComputePlane deployment flow for ApplicationDefinition placement so the target
tenant namespace is pre-created with required PSA labels, quotas, and ownership
metadata before the HelmRelease can install workloads. Do not rely solely on
spec.install.createNamespace; either provision the namespace explicitly or
enforce equivalent admission gating before workload creation.
Source: MCP tools
| - **ComputePlane not ready when a `placement: ComputePlane` app is created** → the `HelmRelease` waits on the `computeplane-admin-kubeconfig` Secret; Flux surfaces not-ready, as with any dependency ordering. | ||
| - **kubeconfig Secret missing/rotated** → remote apply fails closed (no fallback to local apply); the security-correct behavior. |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show nearby context around the cited lines
file="design-proposals/compute-plane/README.md"
wc -l "$file"
sed -n '170,240p' "$file"
# Search for the key terms across the repo
rg -n --hidden --glob '!**/.git/**' -e 'computeplane-admin-kubeconfig|placement: ComputePlane|hardening readiness|PSA|admission|egress policy|remote apply|ComputePlane' .Repository: cozystack/community
Length of output: 35012
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="design-proposals/compute-plane/README.md"
wc -l "$file"
sed -n '170,240p' "$file"
rg -n --hidden --glob '!**/.git/**' -e 'computeplane-admin-kubeconfig|placement: ComputePlane|hardening readiness|PSA|admission|egress policy|remote apply|ComputePlane' .Repository: cozystack/community
Length of output: 35012
Gate placement on full ComputePlane readiness, not just kubeconfig availability. The computeplane-admin-kubeconfig Secret only gives remote-apply credentials; it does not guarantee the ComputePlane’s PSA/admission/egress hardening has finished reconciling. Make placement: ComputePlane wait on a module-level Ready condition before Flux starts the remote release.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@design-proposals/compute-plane/README.md` around lines 207 - 208, Update the
ComputePlane placement dependency documentation and implementation guidance so
the remote HelmRelease waits for the module-level ComputePlane Ready condition,
not merely the computeplane-admin-kubeconfig Secret. Ensure Flux starts the
remote release only after PSA, admission, and egress hardening reconciliation
completes, while preserving the existing fail-closed behavior for missing or
rotated credentials.
| - **kubeconfig Secret missing/rotated** → remote apply fails closed (no fallback to local apply); the security-correct behavior. | ||
| - **`placement: ComputePlane` app but the module is disabled on the tenant** → reject at admission. Never climb to an ancestor's ComputePlane (Design §7). | ||
| - **GPU exhaustion** → cluster-autoscaler adds GPU nodes up to the configured `maxReplicas`; beyond that the workload pends. | ||
| - **Tenant / module deletion** → remote `HelmRelease`s must be deleted *before* the cluster is deprovisioned (a finalizer on the module blocks teardown until they're cleaned up) — otherwise Flux's HelmRelease finalizers block once the target API is gone. |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
sed -n '180,240p' design-proposals/compute-plane/README.mdRepository: cozystack/community
Length of output: 9979
🏁 Script executed:
rg -n "finalizer|teardown|deprovision|disablement|disable|module.*finalizer|HelmRelease" design-proposals/compute-plane/README.mdRepository: cozystack/community
Length of output: 7702
🏁 Script executed:
sed -n '1,180p' design-proposals/compute-plane/README.mdRepository: cozystack/community
Length of output: 26654
🏁 Script executed:
sed -n '240,360p' design-proposals/compute-plane/README.mdRepository: cozystack/community
Length of output: 2408
Clarify teardown ownership and ordering. The finalizer needs an explicit owner and removal path; otherwise module deletion can hang waiting for cleanup, or the ComputePlane can disappear before Flux deletes the remote HelmReleases. Spell out which existing component clears the finalizer and the fail-closed disable/deprovision sequence.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@design-proposals/compute-plane/README.md` at line 211, Clarify the
tenant/module deletion flow by naming the existing component that owns and
removes the module finalizer, and document the fail-closed
disable-then-deprovision sequence. Explicitly state that remote HelmReleases are
deleted before the ComputePlane or target cluster disappears, and describe the
behavior when cleanup cannot be confirmed so deletion cannot hang or proceed
prematurely.
Timofei Larkin (lllamnyp)
left a comment
Timofei Larkin (lllamnyp)
left a comment
There was a problem hiding this comment.
Review: ComputePlane as an operator-owned module
Approving. The delivery surface has landed in the right place, and the "Alternatives considered" side-by-side against the preset-field model is the clearest statement of the trade-off so far. Two asks below: one is a concrete correction to the design text, the other is a sequencing request I'd like honoured before this is implemented against real tenants.
1. The computeplane release name is an external contract — say so in the doc
apps/kubernetes derives the Kamaji kubeconfig Secret from the Helm release name: packages/apps/kubernetes/templates/helmreleases/*.yaml all reference
kubeConfig:
secretRef:
name: {{ .Release.Name }}-admin-kubeconfig
key: super-admin.svcThe design (and cozystack/cozystack#3280) threads a deliberate needle around this: the module's own Helm release takes the -module suffix so the canonical release name computeplane stays reserved for the cluster HelmRelease, which is what makes Kamaji write computeplane-admin-kubeconfig — the Secret that placement: ComputePlane applications consume via kubeConfig.secretRef.
That makes ComputePlane different in kind from every other tenant module, and the design doesn't currently say so. For etcd, monitoring, seaweedfs, the Helm release name is internal identity — nothing outside the module resolves it by name. For ComputePlane the release name is a public contract, consumed by objects the module never sees (every remote HelmRelease routed in by placement).
Why this matters now rather than later: #39 §4 proposes that the tenant bools "become sugar that provisions the corresponding app instance ... instead of open-coding the module". Today packages/apps/tenant/templates/etcd.yaml open-codes metadata.name: etcd and lets the release name follow. A generic instance-provisioning path decides release names generically — and it would be entirely reasonable for such a path to name the release after the app instance, with no way of knowing that this particular release name is load-bearing in two directions at once.
The failure ladder if it renames:
- Kamaji writes the kubeconfig to a differently-named Secret.
- Every
placement: ComputePlaneHelmRelease'ssecretRefdangles → routed apps stop reconciling. This is the acceptable outcome — it's fail-closed, consistent with the doc's stated behaviour for missing or rotated credentials. - The bad outcome: Helm sees a different release and uninstalls the old one. That release is a Kamaji control plane plus KubeVirt-VM workers plus their PVCs — i.e. tenant data, not a restartable stateless component.
#39 §5 does commit to "rename identity not restart" and "running releases are relabelled, not recreated", so the intent is right. But the same section is candid that idempotency is "authored per script, not framework-enforced" — the safety here is authoring convention, and conventions need the invariant to be written down somewhere the author will find it.
Ask: state in the design (Design §1, next to the two-level packaging) that the release name computeplane is a stable external contract consumed by placement: ComputePlane releases via computeplane-admin-kubeconfig, and must not be renamed by any future repackaging. cozystack/cozystack#3280 already pins release name and kubeconfig Secret in helm-unittest, which is the right enforcement — the proposal text should carry the why so that a later migration author does not have to reverse-engineer it from a test failure.
2. #33 and #39 are complementary — but the rationale here leans on the wrong thing
#39 lists this proposal as a "counter-direction" on the grounds that it extends extra and so "taxes every new module with the rigid extra shape". I don't think that's right, and the reason is worth putting in this doc, because it makes the two proposals compose instead of compete.
#39's capabilities (visibility, cardinality, protection, capability) are fields on ApplicationDefinition — operator-owned registration data, not tenant-editable app fields. Read that way, #39's model requires the structure this proposal already builds, for two independent reasons:
- Capabilities are per-kind, so they cannot discriminate. Expressing ComputePlane's posture by putting capabilities on the
KubernetesAD would apply them to everyKubernetesinstance, including the ordinary tenant-ordered clusters a tenant is supposed to administer. Two postures over one chart therefore need two ADs — two kinds. That is exactly theComputePlanemodule-kind here. - The AD cannot carry the hardening.
ApplicationDefinitionSpec(api/v1alpha1/applicationdefinitions_types.go) hasapplication(kind,openAPISchema, plural, singular),release(chartRef, labels, prefix), anddashboard— no fixed-values facility. The conversion isValues: app.Spec(pkg/registry/apps/application/rest.go:1619): the tenant's spec becomes the values. So operator-fixed values have nowhere to live except a chart — the wrapper this proposal specifies.
The obvious shortcut doesn't rescue it either. There is structural-schema defaulting on that path, so one might hope a narrow openAPISchema with hardened defaults could replace the wrapper chart. It can't: settability and defaultability are the same property. A field declared in the schema can be defaulted and overridden by the tenant; a field absent from the schema cannot be overridden but receives no default either, so the chart's own (unhardened) default applies. "Hardened and unreachable" is not expressible in a schema alone.
So the two-piece structure — own AD for the narrow schema (nodeGroups only), wrapper chart for the operator-fixed values — is what the ApplicationDefinition model forces for this case. It isn't a tax imposed by the extra bucket.
Which is the point: that argument is the durable justification for this design's shape, and it is not the argument the doc currently makes. As written, the case rests on the apps vs extra split ("exactly an extra module shape", "cuts against the apps vs extra split" in Alternatives). #39 proposes to delete that category. If it lands, this proposal's central justification points at a bucket that no longer exists, and the rejection of the preset-field model rests on a premise the project has since removed — even though the engineering is entirely unaffected.
Ask: restate the rationale structurally — "a second ApplicationDefinition because capabilities and schemas are per-kind, plus a wrapper chart because the AD has no values field" — rather than in bucket vocabulary. That argument holds verbatim whether the chart lives in packages/extra/computeplane or packages/apps/computeplane, and it makes #39 a no-op for ComputePlane: a directory move plus visibility: module and cardinality: {scope: tenant, max: 1}.
3. Sequencing: land this with #39, not several days ahead of it
This is the ask I feel strongest about, and it's about cost asymmetry rather than correctness.
While both are proposals, a #33/#39 disagreement is resolved by editing markdown. Once cozystack/cozystack#3280 merges and tenants set computeplane: true, the identical disagreement is resolved by migrating live Kamaji clusters with tenant workloads on them — and §1's rename hazard stops being hypothetical, because there is something to rename.
#39's own framing ("the directory move is a git-level change, not a runtime one") is only true for packages nobody has adopted yet. Every day ComputePlane ships ahead of the fold converts that git-level change into a data-path migration for real installs.
So: co-land, or land this after #39's field model is settled. If it must go first, then §1's invariant is the thing to defend through the fold, and I'd want the ComputePlane migration script called out by name in #39's §5 rather than left to the generic per-module treatment.
None of this changes the substance — the mechanism (remote Flux apply, per-VM boundary, single-tenant, scoped egress) is unchanged and was settled in the first revision, and the operator-owned wrapper is the right home for the posture. Approving on that basis; the asks above are about making the doc's reasoning survive #39 and making sure the packaging change reaches tenants only once.
…rapping apps/kubernetes Supersede the preset-field revision (#27): deliver ComputePlane as a Cozystack-owned tenant module (packages/extra/computeplane) that deploys the ordinary apps/kubernetes chart with operator-fixed values via a source-only PackageSource re-declaration. No new CRD, no new controller, no fields on kind: Kubernetes; the module registers its own thin ComputePlane module-kind. Addressing the review: state the computeplane release name as a stable external contract (renaming it would at best dangle every placement: ComputePlane secretRef and at worst have Helm uninstall a live Kamaji cluster), and ground the design's shape structurally — a second ApplicationDefinition because AD properties are per-kind, plus a wrapper chart because the AD carries no fixed-values facility — rather than in the apps-vs-extra directory split, so the rationale composes with #39's capability-field model tracked as a separate stream. Assisted-By: Claude Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
90f0409 to
0ee8b82
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@design-proposals/compute-plane/README.md`:
- Around line 108-121: Define an operator-only namespace and explicit RBAC deny
boundaries for the computeplane-admin-kubeconfig Secret, the module/cluster
HelmRelease containing releaseName computeplane, and their child resources.
Ensure regular tenant subjects in tenant-<name> cannot read, mutate, or delete
these objects, while operator reconciliation and ComputePlane placement access
remain functional. Add helm-unittest or equivalent coverage verifying each
denied access path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: edd88863-2b0e-4c22-8f2b-e14b2eddf657
📒 Files selected for processing (1)
design-proposals/compute-plane/README.md
| releaseName: computeplane # → Kamaji Secret computeplane-admin-kubeconfig | ||
| chartRef: | ||
| kind: ExternalArtifact | ||
| name: cozystack-computeplane-application-kubevirt-kubernetes # apps/kubernetes, re-sourced (Design §2) | ||
| namespace: cozy-system | ||
| values: # operator-owned; the tenant cannot edit these | ||
| # hardened posture: restricted PSA + admission, deny egress → management kube-apiserver, | ||
| # scoped per-service egress only, addon set, GPU node groups, autoscaling bounds … | ||
| valuesFrom: | ||
| - kind: Secret | ||
| name: cozystack-values | ||
| ``` | ||
|
|
||
| **The release name `computeplane` is a stable external contract.** For every other tenant module (`etcd`, `monitoring`, `seaweedfs`) the Helm release name is *internal identity* — nothing outside the module resolves it by name. Here it is consumed by objects the module never sees: `apps/kubernetes` derives the Kamaji admin-kubeconfig Secret from its release name (`<release>-admin-kubeconfig`, key `super-admin.svc`), and every `placement: ComputePlane` HelmRelease references `computeplane-admin-kubeconfig` by that fixed name. No future repackaging, migration, or generic module-provisioning path may rename this release. A rename would, at best, dangle every routed app's `secretRef` (fail-closed, consistent with the missing-credential behaviour below); at worst, Helm treats the cluster as a *different release* and uninstalls the old one — a live Kamaji control plane plus KubeVirt-VM workers and their PVCs, i.e. tenant data. The implementation pins the release name and Secret name in helm-unittest; this paragraph is the *why* behind those pins. |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Protect the admin kubeconfig and backing resources from tenant access.
The design places the cluster HelmRelease in tenant-<name> and makes computeplane-admin-kubeconfig the routing contract, while claiming the tenant never receives that credential. It does not define an operator-only Secret namespace or RBAC preventing regular tenant subjects from reading the Secret or mutating the backing HelmRelease.
If tenant namespace access includes Secret or HelmRelease permissions, users can recover super-admin.svc and deploy cluster-scoped resources, defeating the tamper-proof hardening. Define and test explicit deny boundaries for the Secret, module/cluster HelmReleases, and their child resources.
Also applies to: 180-190
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@design-proposals/compute-plane/README.md` around lines 108 - 121, Define an
operator-only namespace and explicit RBAC deny boundaries for the
computeplane-admin-kubeconfig Secret, the module/cluster HelmRelease containing
releaseName computeplane, and their child resources. Ensure regular tenant
subjects in tenant-<name> cannot read, mutate, or delete these objects, while
operator reconciliation and ComputePlane placement access remain functional. Add
helm-unittest or equivalent coverage verifying each denied access path.
2 participants
What
Revises the ComputePlane design proposal. ComputePlane is delivered as a Cozystack-owned Tenant module (
packages/extra/computeplane) that provisions a hardenedkind: Kubernetesby wrapping the existingapps/kuberneteschart with operator-fixed values — not user-facing preset fields (isolationProfile×componentProfile) onkind: Kubernetes.Because a Tenant module is registered via an
ApplicationDefinition, ComputePlane does get its ownapps.cozystack.iomodule-kind and API (likeSeaweedFS,Kubernetesand every managed service). The accurate framing is therefore: no new CRD, no new controller/reconcile path, and no new fields onkind: Kubernetes— theComputePlanekind is a thin operator-owned wrapper over the unchangedapps/kubernetes.Why this supersedes #27
#27 put the posture on
kind: Kubernetesas user-facing fields. This revision moves it into an operator-ownedextramodule:appsvsextra(docs(development): explain why packages live in apps vs extra website#594): a ComputePlane is a per-tenant enabler switched on under the hood, not a first-class cluster the tenant orders/configures — exactly anextramodule (likeextra/etcd→apps/kubernetes).kind: Kubernetesuntouched: no fields added to the cluster app; the user still gets the same cluster — they just don't administer it and can change only the curated knobs the module exposes.Mechanism
Reuses the existing PackageSource "source-only chart" pattern (how NATS/SeaweedFS already ship): a
computeplane-applicationPackageSource re-sourcesapps/kubernetesas a source-onlyExternalArtifact, plus acomputeplane-rdresource-definition and theextra/computeplanemodule chart. Apps route in via a newplacement: ManagementPlane | ComputePlanefield onApplicationDefinition, injectingkubeConfig.secretRef → computeplane-admin-kubeconfigfor one-way remote Flux apply (unchanged mechanism from the merged first revision).Relationship to prior proposals
Departs from #26's "no new kind at all" (that is the #27 field model, set aside here) but keeps the #26 / #17-review objection to a heavyweight distinct kind: the
ComputePlanemodule-kind does not duplicate the cluster reconcile/RBAC path — it wraps the unchangedapps/kubernetes. Supersedes #27 (preset fields — see "Alternatives considered" for the full trade-off). The underlying isolation mechanism (remote Flux apply, per-VM boundary, single-tenant, scoped egress, cozystack-api visibility control) is unchanged.Closes #26
Summary by CodeRabbit