design-proposals: fold extra into apps — tenant modules as apps with declarative capabilities#39
Draft
myasnikovdaniil wants to merge 1 commit into
Draft
design-proposals: fold extra into apps — tenant modules as apps with declarative capabilities#39myasnikovdaniil wants to merge 1 commit into
myasnikovdaniil wants to merge 1 commit into
Conversation
|
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 to retire the extra package bucket in Cozystack and fold its functionality into apps using declarative capabilities on ApplicationDefinition (such as visibility, cardinality, protection, and shared-provider capabilities). The review feedback highlights several critical areas for improvement in the design: ensuring the capability propagation logic respects intermediate overrides, explicitly detailing the cleanup and re-evaluation process when a capability provider is deleted, recommending the use of finalizers over validating webhooks for deletion protection, and suggesting asynchronous cardinality enforcement in the HelmRelease reconciler to prevent GitOps pipelines from getting stuck.
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.
| 1. An app instance with `capability.provides: [monitoring]` created in a tenant namespace, marked shareable (`capability.share: children`, §2), causes the platform to set on the tenant's namespace `namespace.cozystack.io/capability.monitoring: <tenantName>` and to propagate it into descendant tenants' `_namespace.capability.monitoring` — **the same label + `_namespace` propagation that exists today**, keyed by capability class instead of a hardcoded name. One honest difference: the *resolution algorithm* is unchanged, but the *writer* moves — from Helm render time into a controller. The writer specification below pins down who writes what. | ||
| 2. A consumer app with `capability.consumes: [monitoring]` resolves the nearest ancestor provider from `_namespace.capability.monitoring`, with the identical nearest-ancestor-inclusive fallback (child's own provider wins; otherwise the parent's; otherwise none → the existing `awaiting-*` beacon pattern, cf. `apps/kubernetes`'s `awaiting-etcd`). | ||
|
|
||
| **Writer specification.** Today's writer is render-time: `tenant/templates/namespace.yaml` computes the five provider names from the Tenant chart's bools plus the parent's `_namespace.*`, stamps them as `namespace.cozystack.io/<module>` labels on the child Namespace, and writes them into the child's `cozystack-values` Secret (labelled `reconcile.fluxcd.io/watch: Enabled`); every app HelmRelease has `valuesFrom: cozystack-values` pinned by `cozystack-api` (`pkg/registry/apps/application/rest.go`) and by the ApplicationDefinition helm reconciler (`internal/controller/applicationdefinition_helmreconciler.go`), so a Secret change re-reconciles the consumers. There is no tenant reconciler in Go today, and while render time *can* observe cluster state (`namespace.yaml` already `lookup`s the parent Namespace for its ownerReference uid), it cannot *fire*: creating a provider instance in tenant X touches nothing that any descendant tenant's release watches, so a lookup-based read-back through the existing `_namespace.*` path would converge only on the periodic reconcile interval — staleness unbounded and multiplied per tree level — and would render empty under helm-unittest. The new trigger needs a controller. **The owner is a new capability reconciler in the existing `cozystack-controller`** (`cmd/cozystack-controller`) — the binary that already hosts the tenant-namespace-label-adjacent reconcilers (`internal/controller/tenantgateway` consumes `namespace.cozystack.io/gateway` and renders cross-namespace ReferenceGrants; `tenantquota`; and the ApplicationDefinition reconcilers already watch both `ApplicationDefinition`s and app HelmReleases). *Trigger → write → propagate:* watch app HelmReleases (already labelled `apps.cozystack.io/application.kind`), join to the owning `ApplicationDefinition`'s `capability.provides` + `share: children`; when a provider instance exists in tenant *X*, SSA-apply `namespace.cozystack.io/capability.<class>: X` on *X*'s namespace and on every descendant tenant namespace without a nearer provider — "descendants of *X*" is a plain label selector, because every tenant namespace already carries its full ancestor chain as `tenant.cozystack.io/<ancestor>` labels (`tenant.ancestorTenantLabels`) — and write the matching `_namespace.capability.*` values. *Values vehicle:* the controller must **not** co-write `cozystack-values` — its `_namespace.*` lives inside the single `values.yaml` stringData key that the Tenant chart re-renders wholesale, so two writers of one field would fight on every tenant upgrade. It owns a second, controller-managed Secret per tenant namespace instead (working name `cozystack-capability-values`), likewise labelled `reconcile.fluxcd.io/watch: Enabled` and appended as an optional `valuesFrom` at the same two injection points — descendant releases re-resolve on change through the identical Flux watch mechanism. Those two injection points are verified single-writer chokepoints, which makes the append well-defined: `rest.go` hardcodes `ValuesFrom: [{Secret cozystack-values}]` on every HelmRelease it emits, and the helm reconciler's `expectedValuesFrom()` + `valuesFromEqual` actively *revert* any HelmRelease whose `valuesFrom` list diverges from the expected one (the comparison covers `Optional` too) — so this is a code change at exactly those two sites, after which the same enforcement guarantees every app HelmRelease carries both references, and nothing else can add or strip entries. Merge order is safe by helm-controller's composition rules: `valuesFrom` entries deep-merge in list order (later wins), then inline `spec.values` merges last — so the capability Secret, second in the list, nests `_namespace.capability.*` under `_namespace` alongside the legacy keys without touching them (only an identical leaf key could collide, which is exactly why the capability keyspace is `capability.<class>` rather than reusing `_namespace.<module>`), and the user's own app values keep their existing precedence over both Secrets (`Values: app.Spec` in `rest.go` — inline values already outrank `cozystack-values` today, unchanged trust property). Note the Secret is load-bearing as the *trigger*, not just the vehicle: the `reconcile.fluxcd.io/watch: Enabled` label is what re-reconciles descendant releases when a value changes — a namespace label alone re-renders nothing, which is why "consumers read the capability label directly" is not a simpler alternative. Net effect on the consumer contract: consumers keep the exact `.Values._namespace.*` read pattern; only the key gains a `capability.` segment, and during the compat window both keys resolve. *Who wins during the compat window:* nobody has to — ownership is disjoint, not merged. The Tenant chart (helm-controller's field manager) stays the sole writer of the legacy label keys (`namespace.cozystack.io/monitoring`, …) and of `cozystack-values`; the capability reconciler (its own SSA field manager) is the sole writer of the `namespace.cozystack.io/capability.*` keys and of its own Secret. Disjoint label keys on the Namespace and disjoint Secret objects mean the two field managers never contend, and a Tenant-chart upgrade cannot strip labels it never owned; phase 4 is simply the Tenant chart ceasing to emit the legacy keys. (Home: **recommended `cozystack-controller`** — confirm with maintainers, but the evidence is one-sided rather than taste. That binary already registers every watch surface this reconciler needs (`cmd/cozystack-controller/main.go`): the ApplicationDefinition reconcilers that manage app HelmReleases by the `apps.cozystack.io/application.*` labels, and `tenantgateway`, which already does the exact shape of write proposed here — SSA-patching a `namespace.cozystack.io/*` label onto tenant namespaces (`ensureNamespaceLabels` / `patchNamespaceGatewayLabel`, `internal/controller/tenantgateway/reconciler.go`). `cozystack-operator` registers only the PackageSource/Package reconcilers plus `cozyvaluesreplicator`, whose target selector is `cozystack.io/system=true` (`cmd/cozystack-operator/main.go`) — platform bootstrap and system-namespace scope, with no tenant-namespace, ApplicationDefinition, or app-HelmRelease watches. The capability write path is tenantgateway-shaped, not replicator-shaped.) |
There was a problem hiding this comment.
The propagation logic needs to handle intermediate overrides carefully. If the controller uses a plain label selector (e.g., tenant.cozystack.io/X: "true") to find all descendants of namespace.cozystack.io/capability.<class>: X, it will overwrite descendant namespaces that have a "nearer" provider
| 1. An app instance with `capability.provides: [monitoring]` created in a tenant namespace, marked shareable (`capability.share: children`, §2), causes the platform to set on the tenant's namespace `namespace.cozystack.io/capability.monitoring: <tenantName>` and to propagate it into descendant tenants' `_namespace.capability.monitoring` — **the same label + `_namespace` propagation that exists today**, keyed by capability class instead of a hardcoded name. One honest difference: the *resolution algorithm* is unchanged, but the *writer* moves — from Helm render time into a controller. The writer specification below pins down who writes what. | ||
| 2. A consumer app with `capability.consumes: [monitoring]` resolves the nearest ancestor provider from `_namespace.capability.monitoring`, with the identical nearest-ancestor-inclusive fallback (child's own provider wins; otherwise the parent's; otherwise none → the existing `awaiting-*` beacon pattern, cf. `apps/kubernetes`'s `awaiting-etcd`). | ||
|
|
||
| **Writer specification.** Today's writer is render-time: `tenant/templates/namespace.yaml` computes the five provider names from the Tenant chart's bools plus the parent's `_namespace.*`, stamps them as `namespace.cozystack.io/<module>` labels on the child Namespace, and writes them into the child's `cozystack-values` Secret (labelled `reconcile.fluxcd.io/watch: Enabled`); every app HelmRelease has `valuesFrom: cozystack-values` pinned by `cozystack-api` (`pkg/registry/apps/application/rest.go`) and by the ApplicationDefinition helm reconciler (`internal/controller/applicationdefinition_helmreconciler.go`), so a Secret change re-reconciles the consumers. There is no tenant reconciler in Go today, and while render time *can* observe cluster state (`namespace.yaml` already `lookup`s the parent Namespace for its ownerReference uid), it cannot *fire*: creating a provider instance in tenant X touches nothing that any descendant tenant's release watches, so a lookup-based read-back through the existing `_namespace.*` path would converge only on the periodic reconcile interval — staleness unbounded and multiplied per tree level — and would render empty under helm-unittest. The new trigger needs a controller. **The owner is a new capability reconciler in the existing `cozystack-controller`** (`cmd/cozystack-controller`) — the binary that already hosts the tenant-namespace-label-adjacent reconcilers (`internal/controller/tenantgateway` consumes `namespace.cozystack.io/gateway` and renders cross-namespace ReferenceGrants; `tenantquota`; and the ApplicationDefinition reconcilers already watch both `ApplicationDefinition`s and app HelmReleases). *Trigger → write → propagate:* watch app HelmReleases (already labelled `apps.cozystack.io/application.kind`), join to the owning `ApplicationDefinition`'s `capability.provides` + `share: children`; when a provider instance exists in tenant *X*, SSA-apply `namespace.cozystack.io/capability.<class>: X` on *X*'s namespace and on every descendant tenant namespace without a nearer provider — "descendants of *X*" is a plain label selector, because every tenant namespace already carries its full ancestor chain as `tenant.cozystack.io/<ancestor>` labels (`tenant.ancestorTenantLabels`) — and write the matching `_namespace.capability.*` values. *Values vehicle:* the controller must **not** co-write `cozystack-values` — its `_namespace.*` lives inside the single `values.yaml` stringData key that the Tenant chart re-renders wholesale, so two writers of one field would fight on every tenant upgrade. It owns a second, controller-managed Secret per tenant namespace instead (working name `cozystack-capability-values`), likewise labelled `reconcile.fluxcd.io/watch: Enabled` and appended as an optional `valuesFrom` at the same two injection points — descendant releases re-resolve on change through the identical Flux watch mechanism. Those two injection points are verified single-writer chokepoints, which makes the append well-defined: `rest.go` hardcodes `ValuesFrom: [{Secret cozystack-values}]` on every HelmRelease it emits, and the helm reconciler's `expectedValuesFrom()` + `valuesFromEqual` actively *revert* any HelmRelease whose `valuesFrom` list diverges from the expected one (the comparison covers `Optional` too) — so this is a code change at exactly those two sites, after which the same enforcement guarantees every app HelmRelease carries both references, and nothing else can add or strip entries. Merge order is safe by helm-controller's composition rules: `valuesFrom` entries deep-merge in list order (later wins), then inline `spec.values` merges last — so the capability Secret, second in the list, nests `_namespace.capability.*` under `_namespace` alongside the legacy keys without touching them (only an identical leaf key could collide, which is exactly why the capability keyspace is `capability.<class>` rather than reusing `_namespace.<module>`), and the user's own app values keep their existing precedence over both Secrets (`Values: app.Spec` in `rest.go` — inline values already outrank `cozystack-values` today, unchanged trust property). Note the Secret is load-bearing as the *trigger*, not just the vehicle: the `reconcile.fluxcd.io/watch: Enabled` label is what re-reconciles descendant releases when a value changes — a namespace label alone re-renders nothing, which is why "consumers read the capability label directly" is not a simpler alternative. Net effect on the consumer contract: consumers keep the exact `.Values._namespace.*` read pattern; only the key gains a `capability.` segment, and during the compat window both keys resolve. *Who wins during the compat window:* nobody has to — ownership is disjoint, not merged. The Tenant chart (helm-controller's field manager) stays the sole writer of the legacy label keys (`namespace.cozystack.io/monitoring`, …) and of `cozystack-values`; the capability reconciler (its own SSA field manager) is the sole writer of the `namespace.cozystack.io/capability.*` keys and of its own Secret. Disjoint label keys on the Namespace and disjoint Secret objects mean the two field managers never contend, and a Tenant-chart upgrade cannot strip labels it never owned; phase 4 is simply the Tenant chart ceasing to emit the legacy keys. (Home: **recommended `cozystack-controller`** — confirm with maintainers, but the evidence is one-sided rather than taste. That binary already registers every watch surface this reconciler needs (`cmd/cozystack-controller/main.go`): the ApplicationDefinition reconcilers that manage app HelmReleases by the `apps.cozystack.io/application.*` labels, and `tenantgateway`, which already does the exact shape of write proposed here — SSA-patching a `namespace.cozystack.io/*` label onto tenant namespaces (`ensureNamespaceLabels` / `patchNamespaceGatewayLabel`, `internal/controller/tenantgateway/reconciler.go`). `cozystack-operator` registers only the PackageSource/Package reconcilers plus `cozyvaluesreplicator`, whose target selector is `cozystack.io/system=true` (`cmd/cozystack-operator/main.go`) — platform bootstrap and system-namespace scope, with no tenant-namespace, ApplicationDefinition, or app-HelmRelease watches. The capability write path is tenantgateway-shaped, not replicator-shaped.) |
There was a problem hiding this comment.
The design should explicitly specify how the controller handles the deletion of a capability provider. When a provider in tenant namespace.cozystack.io/capability.<class> label from cozystack-capability-values Secret accordingly to trigger re-reconciliation of the consumers.\n\nWithout this cleanup and re-evaluation, consumers will continue to reference the deleted provider, leading to broken connections.
|
|
||
| - Model tenant singletons as `cardinality.scope: tenant, max: 1`, or introduce a distinct "tenant service" flavour? (Prefer the former — fewer concepts.) | ||
| - Keep the Tenant bools as permanent ergonomic sugar, or force explicit app creation after phase 4? | ||
| - `protection`: finalizer vs validating webhook vs RBAC-only — which enforcement? |
There was a problem hiding this comment.
For protection enforcement, a finalizer is highly recommended over a validating webhook or RBAC-only rules.\n\nValidating webhooks can be bypassed under certain cluster administration conditions or during API server disruptions, and RBAC-only rules do not prevent accidental deletions by authorized administrators or automated service accounts. A finalizer ensures that the resource cannot be physically deleted from the cluster until all active references (e.g., child tenant dependencies) are verified to be gone, preventing dangling references and broken states.
| - Model tenant singletons as `cardinality.scope: tenant, max: 1`, or introduce a distinct "tenant service" flavour? (Prefer the former — fewer concepts.) | ||
| - Keep the Tenant bools as permanent ergonomic sugar, or force explicit app creation after phase 4? | ||
| - `protection`: finalizer vs validating webhook vs RBAC-only — which enforcement? | ||
| - Cardinality enforcement point: `cozystack-api` admission only sees instances created through the aggregated API; the Tenant sugar (§4) and hand-authored `HelmRelease`s do not pass through it. Enforce additionally at the `HelmRelease` level (validating webhook / policy), or treat platform-rendered releases as trusted and document the gap? |
There was a problem hiding this comment.
For the cardinality enforcement point, using a validating webhook on HelmRelease can be problematic for GitOps workflows (e.g., Flux), as blocking the admission of a HelmRelease can cause the GitOps pipeline to get stuck.\n\nA more GitOps-friendly approach is to enforce this asynchronously in the HelmRelease reconciler (within cozystack-controller). If a HelmRelease violates the cardinality constraint defined in its ApplicationDefinition, the reconciler can mark the HelmRelease status as Failed or Errored with a clear message, rather than blocking the API write.
Retire the packages/extra bucket by folding those packages into apps and expressing hidden-from-catalog / per-tenant-singleton / shared-down-the-tree as declarative capabilities on ApplicationDefinition (visibility, cardinality, protection, capability with share). Supersedes community#4 (tenant module overrides); complementary to community#25 (per-cluster etcd). Grounded against cozystack main @ cece5c23b. Assisted-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
myasnikovdaniil
force-pushed
the
proposal/fold-extra-into-apps
branch
from
1836bc8 to
5ab45ec
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Retire the
packages/extrabucket. Every package underpackages/extrabecomes a regularappspackage (or moves tocore/system), and the three things that actually distinguish anextrapackage today — hidden from the catalog, at most one per tenant, and shared down the tenant tree — become declarative capabilities on anyApplicationDefinition(visibility,cardinality,protection,capabilitywithshare), not a directory boundary.extrais not a distinct runtime mechanism: anextrapackage uses the samePackageSourceregistration, the same chart patterns, and the sameApplicationDefinitionas anappspackage. The differences are a few dashboard/release flags and the way the Tenant chart wires provider discovery. Expressed as data, the second bucket disappears and any app can opt into per-tenant-singleton, hidden, or shared-provider behaviour.Relationship to other proposals
@embed/valuesOverridelayer is unnecessary once modules are first-class apps with opt-in capabilities; this proposal will close design-proposals: tenant module overrides #4 as Superseded.extramodule). design-proposal(compute-plane): deliver as an operator-owned extra module wrapping apps/kubernetes (supersedes #27) #33 is the live pull toward extendingextra; this proposal argues that direction taxes every new module with the rigid extra shape. Sequencing to be coordinated.Status
Draft PR — opening for early visibility while the open questions settle. Grounded against
cozystackmain @cece5c23b; every code claim (registration parity, tenant-chart provider resolution, console filter, migration mechanism, writer path) was verified against the source. Open questions are listed in the doc.